Commit f357233c assumed that it was OK to return ENOENT directly if
lstat() failed that way. If we got STATUS_DELETE_PENDING while trying
to unlink a file that we had already unlinked successfully once before
but someone else still had open (on a kernel version that has "pending"
unlinks by default), then we would no longer reach the retry loop in
pgunlink(). That loop claims to be only for handling sharing violations
(a different phenomenon), but the errno is the same.
Restore that behavior with an explicit check, to see if it fixes the
occasional 'directory not empty' failures seen in the pg_upgrade tests
on CI. Further improvements are possible with proposed upgrades to
modern Windows APIs that would replace this convoluted code.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20220920013122.GA31833%40telsasoft.com
Discussion: https://postgr.es/m/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
Commit c5cb8f3b supposed that we'd only ever have to follow one junction
point in stat(), because we don't construct longer chains of them ourselves.
When examining a parent directory supplied by the user, we should really be
able to cope with longer chains, just in case someone has their system
set up that way. Choose an arbitrary cap of 8, to match the minimum
acceptable value of SYMLOOP_MAX in POSIX.
Previously I'd avoided reporting ELOOP thinking Windows didn't have it,
but it turns out that it does, so we can use the proper error number.
Reviewed-by: Roman Zharkov <r.zharkov@postgrespro.ru>
Discussion: https://postgr.es/m/CA%2BhUKGJ7JDGWYFt9%3D-TyJiRRy5q9TtPfqeKkneWDr1XPU1%2Biqw%40mail.gmail.com
Discussion: https://postgr.es/m/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
Since commit c5cb8f3b taught stat() to follow symlinks, and since initdb
uses pg_mkdir_p(), and that examines parent directories, our humble
readlink() implementation can now be exposed to junction points not of
PostgreSQL origin. Those might be corrupted by our naive path mangling,
which doesn't really understand NT paths in general.
Simply decline to transform paths that don't look like a drive absolute
path. That means that readlink() returns the NT path directly when
checking a parent directory of PGDATA that happen to point to a drive
using "rooted" format. That works for the purposes of our stat()
emulation.
Reported-by: Roman Zharkov <r.zharkov@postgrespro.ru>
Reviewed-by: Roman Zharkov <r.zharkov@postgrespro.ru>
Discussion: https://postgr.es/m/4590c37927d7b8ee84f9855d83229018%40postgrespro.ru
Discussion: https://postgr.es/m/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
When using junction points to emulate symlinks on Windows, one edge case
was not handled correctly by commit c5cb8f3b: if a junction point is
broken (pointing to a non-existent path), we'd report ENOENT. This
doesn't break any known use case, but was noticed while developing a
test suite for these functions and is fixed here for completeness.
Also add translation ERROR_CANT_RESOLVE_FILENAME -> ENOENT, as that is
one of the errors Windows can report for some kinds of broken paths.
Discussion: https://postgr.es/m/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
Commit df3737a651 added an incorrect assertion about the preconditions
for invoking the backup cleanup callback: it misfires at session end in
case a backup completes successfully. Fix it, using coding from Michaël
Paquier. Also add some tests for the various cases.
Reported by Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/20221021.161038.1277961198945653224.horikyota.ntt@gmail.com
While looking at how these are handled in the parser and the executor, I
have noticed that there is no test coverage for most of these when
reverse-engineering an expression for a SQLValueFunction node in
ruleutils.c, including how these are reparsed when included in a FROM
clause. Some hacking in this area has showed me that these could break
easily, so add some coverage to track the existing compatibility.
Extracted from a much larger patch by me.
Discussion: https://postgr.es/m/YzaG3MoryCguUOym@paquier.xyz
The new tests have been reporting a warning hidden in the logs, as of
"Odd number of elements in hash assignment" (perlcritic or similar did
not report an issue, actually). This comes down to a typo in the test
"matching regexp for username" for a double-quoted regexp using commas,
where we passed an extra argument. The test is intended to pass, but
this was causing the test to fail. This also pointed out that the
newly-added role "md5,role" lacks an entry in the password file used to
provide the password, so add one.
While on it, make the tests pickier by checking the contents of the logs
generated on successful authentication.
Oversights in 8fea868.
As of this commit, any database or user entry beginning with a slash (/)
is considered as a regular expression. This is particularly useful for
users, as now there is no clean way to match pattern on multiple HBA
lines. For example, a user name mapping with a regular expression needs
first to match with a HBA line, and we would skip the follow-up HBA
entries if the ident regexp does *not* match with what has matched in
the HBA line.
pg_hba.conf is able to handle multiple databases and roles with a
comma-separated list of these, hence individual regular expressions that
include commas need to be double-quoted.
At authentication time, user and database names are now checked in the
following order:
- Arbitrary keywords (like "all", the ones beginning by '+' for
membership check), that we know will never have a regexp. A fancy case
is for physical WAL senders, we *have* to only match "replication" for
the database.
- Regular expression matching.
- Exact match.
The previous logic did the same, but without the regexp step.
We have discussed as well the possibility to support regexp pattern
matching for host names, but these happen to lead to tricky issues based
on what I understand, particularly with host entries that have CIDRs.
This commit relies heavily on the refactoring done in a903971 and
fc579e1, so as the amount of code required to compile and execute
regular expressions is now minimal. When parsing pg_hba.conf, all the
computed regexps needs to explicitely free()'d, same as pg_ident.conf.
Documentation and TAP tests are added to cover this feature, including
cases where the regexps use commas (for clarity in the docs, coverage
for the parsing logic in the tests).
Note that this introduces a breakage with older versions, where a
database or user name beginning with a slash are treated as something to
check for an equal match. Per discussion, we have discarded this as
being much of an issue in practice as it would require a cluster to
have database and/or role names that begin with a slash, as well as HBA
entries using these. Hence, the consistency gained with regexps in
pg_ident.conf is more appealing in the long term.
**This compatibility change should be mentioned in the release notes.**
Author: Bertrand Drouvot
Reviewed-by: Jacob Champion, Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/fff0d7c1-8ad4-76a1-9db3-0ab6ec338bf7@amazon.com
It's unclear why a separate type would be needed here. We use plain
pid_t (or int) everywhere else.
(The only relevant platform where pid_t is not int is 64-bit MinGW,
where it is long long int. So defining pid_t as long (which is 32-bit
on Windows), as was done here, doesn't even accommodate that one.)
Reverts 66fa6eba5a.
Discussion: https://www.postgresql.org/message-id/289c2e45-c7d9-5ce4-7eff-a9e2a33e1580@enterprisedb.com
Because of a small thinko in 7844c9918a,
psql -c would exit successfully when a query is canceled. Fix this so
that it exits with a nonzero status, just like for all other errors.
Since pg_backup_start() and pg_backup_stop() exist, the tablespace map
data and the backup state data (backup_label string until 7d70809) have
been allocated in the TopMemoryContext. This approach would cause
memory leaks in the session calling these functions if failures happen
before pg_backup_stop() ends, leaking more memory on repeated failures.
Both things need little memory so that would not be really noticeable
for most users, except perhaps connection poolers with long-lived
connections able to trigger backup failures with these functions.
This commit improves the logic in this area by not allocating anymore
the backup-related data that needs to travel across the SQL-callable
backup functions in TopMemoryContext, by using instead a dedicated
memory context child of TopMemoryContext. The memory context is created
in pg_backup_start() and deleted when finishing pg_backup_stop(). In
the event of an in-flight failure, this memory context gets reset in the
follow-up pg_backup_start() call, so as we are sure that only one run
worth of data is leaked at any time. Some cleanup was already done for
the backup data on a follow-up call of pg_backup_start(), but using a
memory context makes the whole simpler.
BASE_BACKUP commands are executed in isolation, relying on the memory
context created for replication commands, hence these do not need such
an extra logic.
Author: Bharath Rupireddy
Reviewed-by: Robert Haas, Alvaro Herrera, Cary Huang, Michael Paquier
Discussion: https://postgr.es/m/CALj2ACXqvfKF2B0beQ=aJMdWnpNohmBPsRg=EDQj_6y1t2O8mQ@mail.gmail.com
Specifically, when pg_basebackup is invoked with -Tx=y, don't error
out if x could plausibly be an absolute path either on Windows or on
non-Windows systems. We don't know whether the remote system is
running the same OS as the local system, so it's not appropriate to
assume that our local rule about absolute pathnames is the same as
the rule on the remote system.
Patch by me, reviewed by Tom Lane, Andrew Dunstan, and
Davinder Singh.
Discussion: http://postgr.es/m/CA+TgmoY+jC3YiskomvYKDPK3FbrmsDU7_8+wMHt02HOdJeRb0g@mail.gmail.com
It happens that the parts of hba.conf that are planned to be extended
to support regular expressions would finish by using the same error
message as the one used currently for pg_ident.conf when a regular
expression cannot be compiled, as long as the routine centralizing the
logic, regcomp_auth_token(), knows from which file the regexp comes from
and its line location in the so-said file.
This change makes the follow-up patches slightly simpler, and the logic
remains the same. I suspect that this makes the proposal to add support
for file inclusions in pg_ident.conf and pg_hba.conf slightly simpler,
as well.
Extracted from a larger patch by the same author. This is similar to
the refactoring done in fc579e1.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/fff0d7c1-8ad4-76a1-9db3-0ab6ec338bf7@amazon.com
Various test suites use the "openssl" program as part of their setup.
There isn't a way to override which openssl program is to be used,
other than by fiddling with the path, perhaps. This has gotten
increasingly problematic because different versions of openssl have
different capabilities and do different things by default.
This patch checks for an openssl binary in configure and meson setup,
with appropriate ways to override it. This is similar to how "lz4"
and "zstd" are handled, for example. The meson build system actually
already did this, but the result was only used in some places. This
is now applied more uniformly.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/dc638b75-a16a-007d-9e1c-d16ed6cf0ad2%40enterprisedb.com
This makes the choice of result scale of numeric power() for integer
exponents consistent with the choice for non-integer exponents, and
with the result scale of other numeric functions. Specifically, the
result scale will be at least as large as the scale of either input,
and sufficient to ensure that the result has at least 16 significant
digits.
Formerly, the result scale was based only on the scale of the first
input, without taking into account the weight of the result. For
results with negative weight, that could lead to results with very few
or even no non-zero significant digits (e.g., 10.0 ^ (-18) produced
0.0000000000000000).
Fix this by moving responsibility for the choice of result scale into
power_var_int(), which already has code to estimate the result weight.
Per report by Adrian Klaver and suggested fix by Tom Lane.
No back-patch -- arguably this is a bug fix, but one which is easy to
work around, so it doesn't seem worth the risk of changing query
results in stable branches.
Discussion: https://postgr.es/m/12a40226-70ac-3a3b-3d3a-fdaf9e32d312%40aklaver.com
When the logical decoding restarts from NEW_CID, since there is no
association between the top transaction and its subtransaction, both are
created as top transactions and have the same LSN. This caused the
assertion failure in AssertTXNLsnOrder().
This patch skips the assertion check until we reach the LSN at which we
start decoding the contents of the transaction, specifically
start_decoding_at LSN in SnapBuild. This is okay because we don't
guarantee to make the association between top transaction and
subtransaction until we try to decode the actual contents of transaction.
The ordering of the records prior to the start_decoding_at LSN should have
been checked before the restart.
The other assertion failure is due to the reason that we forgot to track
that we have considered top-level transaction id in the list of catalog
changing transactions that were committed when one of its subtransactions
is marked as containing catalog change.
Reported-by: Tomas Vondra, Osumi Takamichi
Author: Masahiko Sawada, Kuroda Hayato
Reviewed-by: Amit Kapila, Dilip Kumar, Kuroda Hayato, Kyotaro Horiguchi, Masahiko Sawada
Backpatch-through: 10
Discussion: https://postgr.es/m/a89b46b6-0239-2fd5-71a9-b19b1f7a7145%40enterprisedb.com
Discussion: https://postgr.es/m/TYCPR01MB83733C6CEAE47D0280814D5AED7A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Set up a signal handler for INT/TERM so that we run our END block if we
get them. In END, if the exit status indicates a problem, call
_update_pid(-1) to improve chances of the stop working in case start()
hasn't returned yet.
Also, change END's teardown_node() so that it passes fail_ok=>1, so that
if a node fails to stop, we still stop the other nodes in the same test.
Per complaint from Andres Freund.
This doesn't seem important enough to backpatch, at least for now.
Discussion: https://postgr.es/m/20220930040734.mbted42oiynhn2t6@awork3.anarazel.de
After commit 39969e2a1e, ->forcePageWrites is no longer very
interesting: we can just test whether runningBackups is different from 0.
This simplifies some code, so do away with it.
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/39969e2a1e4d7f5a37f3ef37d53bbfe171e7d77a
We had two copies of almost identical logic to revert shared memory
state when a running backup aborts; we can remove
pg_backup_start_callback if we adapt do_pg_abort_backup so that it can
be used for this purpose too.
However, in order for this to work, we have to repurpose the flag passed
to do_pg_abort_backup. It used to indicate whether to throw a warning
(and the only caller always passed true). It now indicates whether the
callback is being called at start time (in which case the session backup
state is known not to have been set to RUNNING yet, so action is always
taken) or shmem time (in which case action is only taken if the session
backup state is RUNNING). Thus the meaning of the flag is no longer
superfluous, but it's actually quite critical to get right. I (Álvaro)
chose to change the polarity and the code flow re. the flag from what
Bharath submitted, for coding clarity.
Co-authored-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://www.postgresql.org/message-id/20221013111330.564fk5tkwe3ha77l%40alvherre.pgsql
As currently designed, with a callback registered in a ERROR_CLEANUP
block, the shutdown callback would get called twice when updating
archive_library on SIGHUP, which is something that we want to avoid to
ease the life of extension writers.
Anyway, an ERROR in the archiver process is treated as a FATAL, stopping
it immediately, hence there is no need for a ERROR_CLEANUP block.
Instead of that, the shutdown callback is not called upon
before_shmem_exit(), giving to the modules the opportunity to do any
cleanup actions before the server shuts down its subsystems.
While on it, this commit adds some testing coverage for the shutdown
callback. Neither shell_archive nor basic_archive have been using it,
and one is added to shell_archive, whose trigger is checked in a TAP
test through a shutdown sequence.
Author: Nathan Bossart, Bharath Rupireddy
Reviewed-by: Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/20221015221328.GB1821022@nathanxps13
Backpatch-through: 15
make_ctags did not include field members of structs since the commit
964d01ae90.
For example, in the following field of RestrictInfo:
Selectivity norm_selec pg_node_attr(equal_ignore);
pg_node_attr was mistakenly interpreted to be the name of the field.
To fix this, add -I option to ctags command if the command is
Exuberant ctags or Universal ctags (for plain old ctags, struct
members are not included in the tags file anyway).
Also add "-e" and "-n" options to make_ctags. The -e option invokes
ctags command with -e option, which produces TAGS file for emacs. This
allows to eliminate duplicate codes in make_etags so that make_etags
just exec make_ctags with -e option.
The -n option allows not to produce symbolic links in each
sub directory (the default is producing symbolic links).
Author: Yugo Nagata
Reviewers: Alvaro Herrera, Tatsuo Ishii
Discussion: https://postgr.es/m/flat/20221007154442.76233afc7c5b255c4de6528a%40sraoss.co.jp
AuthToken gains a regular expression, and IdentLine is changed so as it
uses an AuthToken rather than tracking separately the ident user string
used for the regex compilation and its generated regex_t. In the case
of pg_ident.conf, a set of AuthTokens is built in the pre-parsing phase
of the file, and an extra regular expression is compiled when building
the list of IdentLines, after checking the sanity of the fields in a
pre-parsed entry.
The logic in charge of computing and executing regular expressions is
now done in a new set of routines called respectively
regcomp_auth_token() and regexec_auth_token() that are wrappers around
pg_regcomp() and pg_regexec(), working on AuthTokens. While on it, this
patch adds a routine able to free an AuthToken, free_auth_token(), to
simplify a bit the logic around the requirement of using a specific free
routine for computed regular expressions. Note that there are no
functional or behavior changes introduced by this commit.
The goal of this patch is to ease the use of regular expressions with
more items of pg_hba.conf (user list, database list, potentially
hostnames) where AuthTokens are used extensively. This will be tackled
later in a separate patch.
Author: Bertrand Drouvot, Michael Paquier
Discussion: https://postgr.es/m/fff0d7c1-8ad4-76a1-9db3-0ab6ec338bf7@amazon.com
Preprocessing of the HAVING clause will reduce havingQual to NIL
if the clause is constant-TRUE. This is one case where that
convention is rather unfortunate, because "HAVING TRUE" is not at all
the same as not having any HAVING clause at all. (Per the SQL spec,
it still forces the query to be grouped.) The planner deals with this
by having a boolean hasHavingQual that records whether havingQual was
originally nonempty; places that just want to check whether HAVING
was specified are supposed to consult that.
I found three places that got that wrong. Fortunately, these could
only affect cost estimates not correctness. It'd be hard even
to demonstrate the errors; for example, the one in allpaths.c would
only matter in a query that has HAVING TRUE but no GROUP BY and no
aggregates, which would require a completely variable-free SELECT
list, making the case probably of only academic interest. Hence,
while these are worth fixing before someone copies the incorrect
coding somewhere more critical, they don't seem worth back-patching.
I didn't bother trying to devise regression tests, either.
Discussion: https://postgr.es/m/2503888.1666042643@sss.pgh.pa.us
The original hint says to use SET PUBLICATION when really ADD/DROP
PUBLICATION is called for, so this is arguably a bug fix.
Also, a very similar message elsewhere was using an inconsistent
SQLSTATE.
While at it, unwrap some strings.
Backpatch to 15.
Author: Hou zj <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/OS0PR01MB57160AD0E7386547BA978EB394299@OS0PR01MB5716.jpnprd01.prod.outlook.com
These routines have been renamed in a19e5ce. There is no need to keep
the compatibility declarations on HEAD, as once an extension moves to
the new routine name when compiling with v16~ the code would work the
same way when recompiled on v15. No backpatch to v15 for this one,
because ABI compatibility has to be maintained there.
Discussion: https://postgr.es/m/20221013194820.ciktb2sbbpw7cljm@awork3.anarazel.de
Per discussion, the existing routine name able to initialize a SRF
function with materialize mode is unpopular, so rename it. Equally, the
flags of this function are renamed, as of:
- SRF_SINGLE_USE_EXPECTED -> MAT_SRF_USE_EXPECTED_DESC
- SRF_SINGLE_BLESS -> MAT_SRF_BLESS
The previous function and flags introduced in 9e98583 are kept around
for compatibility purposes, so as any extension code already compiled
with v15 continues to work as-is. The declarations introduced here for
compatibility will be removed from HEAD in a follow-up commit.
The new names have been suggested by Andres Freund and Melanie
Plageman.
Discussion: https://postgr.es/m/20221013194820.ciktb2sbbpw7cljm@awork3.anarazel.de
Backpatch-through: 15
When creating a cast that uses a conversion function, we've
historically allowed the input and result types to be
binary-compatible with the function's input and result types,
rather than necessarily being identical. This means that the new
cast is logically dependent on the binary-compatible cast or casts
that it references: if those are defined by pg_cast entries, and you
try to restore the new cast without having defined them, it'll fail.
Hence, we should make pg_depend entries to record these dependencies
so that pg_dump knows that there is an ordering requirement.
This is not the only place where we allow such shortcuts; aggregate
functions for example are similarly lax, and in principle should gain
similar dependencies. However, for now it seems sufficient to fix
the cast-versus-cast case, as pg_dump's other ordering heuristics
should keep it out of trouble for other object types.
Per report from David Turoň; thanks also to Robert Haas for
preliminary investigation. I considered back-patching, but
seeing that this issue has existed for many years without
previous reports, it's not clear it's worth the trouble.
Moreover, back-patching wouldn't be enough to ensure that the
new pg_depend entries exist in existing databases anyway.
Discussion: https://postgr.es/m/OF0A160F3E.578B15D1-ONC12588DA.003E4857-C12588DA.0045A428@notes.linuxbox.cz
DefineQueryRewrite() has long required that ON SELECT rules be named
"_RETURN". But we overlooked the converse case: we should forbid
non-ON-SELECT rules that are named "_RETURN". In particular this
prevents using CREATE OR REPLACE RULE to overwrite a view's _RETURN
rule with some other kind of rule, thereby breaking the view.
Per bug #17646 from Kui Liu. Back-patch to all supported branches.
Discussion: https://postgr.es/m/17646-70c93cfa40365776@postgresql.org
The executor will dump core if it's asked to execute a seqscan on
a relation having no table AM, such as a view. While that shouldn't
really happen, it's possible to get there via catalog corruption,
such as a missing ON SELECT rule. It seems worth installing a defense
against that. There are multiple plausible places for such a defense,
but I picked the planner's get_relation_info().
Per discussion of bug #17646 from Kui Liu. Back-patch to v12 where
the tableam APIs were introduced; in older versions you won't get a
SIGSEGV, so it seems less pressing.
Discussion: https://postgr.es/m/17646-70c93cfa40365776@postgresql.org
There is already some coverage for that in the kerberos test suite,
though it requires PG_TEST_EXTRA to be set as per its insecure nature.
This provides coverage in a default setup, as long as peer is supported
on the platform where its test is run.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/7f87ca27-e184-29da-15d6-8be4325ad02e@gmail.com
If the non-recursive term of a SEARCH BREADTH FIRST recursive
query has only constants in its target list, the planner will
fold the starting RowExpr added by rewrite into a simple Const
of type RECORD. The executor doesn't have any problem with
that --- but EXPLAIN VERBOSE will encounter the Const as the
ultimate source of truth about what the field names of the
SET column are, and it didn't know what to do with that.
Fortunately, we can pull the identifying typmod out of the
Const, in much the same way that record_out would.
For reasons that remain a bit obscure to me, this only fails
with SEARCH BREADTH FIRST, not SEARCH DEPTH FIRST or CYCLE.
But I added regression test cases for both of those options
too, just to make sure we don't break it in future.
Per bug #17644 from Matthijs van der Vleuten. Back-patch
to v14 where these constructs were added.
Discussion: https://postgr.es/m/17644-3bd1f3036d6d7a16@postgresql.org
In the latest version of Apple's macOS SDK, <sys/socket.h>
fails to compile if "REF" is #define'd as something.
Apple may or may not agree that this is a bug, and even if
they do accept the bug report I filed, they probably won't
fix it very quickly. In the meantime, our back branches will all
fail to compile gram.y. v15 and HEAD currently escape the problem
thanks to the refactoring done in 98e93a1fc, but that's purely
accidental. Moreover, since that patch removed a widely-visible
inclusion of <netdb.h>, back-patching it seems too likely to break
third-party code.
Instead, change the token's code name to REF_P, following our usual
convention for naming parser tokens that are likely to have symbol
conflicts. The effects of that should be localized to the grammar
and immediately surrounding files, so it seems like a safer answer.
Per project policy that we want to keep recently-out-of-support
branches buildable on modern systems, back-patch all the way to 9.2.
Discussion: https://postgr.es/m/1803927.1665938411@sss.pgh.pa.us
snprintf.c has always fallen back on libc's *printf implementation
when printing pointers (%p) and floats. When this code originated,
we were still supporting some platforms that lacked native snprintf,
so we used sprintf for that. That's not actually unsafe in our usage,
but nonetheless builds on macOS are starting to complain about sprintf
being unconditionally deprecated; and I wouldn't be surprised if other
platforms follow suit. There seems little reason to believe that any
platform supporting C99 wouldn't have standards-compliant snprintf,
so let's just use that instead to suppress such warnings.
Back-patch to v12, which is where we started to require C99. It's
also where we started to use our snprintf.c everywhere, so this
wouldn't be enough to suppress the warning in older branches anyway
--- that is, in older branches these aren't necessarily all our
usages of libc's sprintf. It is enough in v12+ because any
deprecation annotation attached to libc's sprintf won't apply to
pg_sprintf. (Whether all our usages of pg_sprintf are adequately
safe is not a matter I intend to address here, but perhaps it could
do with some review.)
Per report from Andres Freund and local testing.
Discussion: https://postgr.es/m/20221015211955.q4cwbsfkyk3c4ty3@awork3.anarazel.de
While directly targetting a foreign table with MERGE was already
expressly forbidden, we failed to catch the case of a partitioned table
that has a foreign table as a partition; and the result if you try is an
incomprehensible error. Fix that by adding a specific check.
Backpatch to 15.
Reported-by: Tatsuhiro Nakamori <bt22nakamorit@oss.nttdata.com>
Discussion: https://postgr.es/m/bt22nakamorit@oss.nttdata.com
It can be useful to know when a relation has last been used, e.g., when
evaluating whether an index is still required. It was already possible to
infer the time of the last usage by tracking, e.g.,
pg_stat_all_indexes.idx_scan over time. But far from everybody does so.
To make it easier to detect the last time a relation has been scanned, track
that time in each relation's pgstat entry. To minimize overhead a) the
timestamp is updated only when the backend pending stats entry is flushed to
shared stats b) the last transaction's stop timestamp is used as the
timestamp.
Bumps catalog and stats format versions.
Author: Dave Page <dpage@pgadmin.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Bruce Momjian <bruce@momjian.us>
Reviewed-by: Vik Fearing <vik@postgresfriends.org>
Discussion: https://postgr.es/m/CA+OCxozrVHNFVEPkweUHMZje+t1tfY816d9MZYc6eZwOOusOaQ@mail.gmail.com
Previously GetCurrentTransactionStopTimestamp() computed a new timestamp
whenever xactStopTimestamp was unset and xactStopTimestamp was only set when a
commit or abort record was written.
An upcoming patch will add additional calls to
GetCurrentTransactionStopTimestamp() from pgstats. To avoid computing
timestamps multiple times, set xactStopTimestamp in
GetCurrentTransactionStopTimestamp() if not already set.
Author: Dave Page <dpage@pgadmin.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Vik Fearing <vik@postgresfriends.org>
Discussion: https://postgr.es/m/20220906155325.an3xesq5o3fq36gt%40awork3.anarazel.de
When a query whose results were requested in single-row mode is the last
in the queue by the time those results are being read, the single-row
flag was not being reset, because we were returning early from
pqPipelineProcessQueue. Move that stanza up so that the flag is always
reset at the end of sending that query's results.
Add a test for the situation.
Backpatch to 14.
Author: Denis Laxalde <denis.laxalde@dalibo.com>
Discussion: https://postgr.es/m/01af18c5-dacc-a8c8-07ee-aecc7650c3e8@dalibo.com
The previous patch made addition of new GUCs cheap, but other GUC
operations aren't improved and indeed get a bit slower, because
hash_seq_search() is slower than just scanning a pointer array.
However, most performance-critical GUC operations only need
to touch a relatively small fraction of the GUCs; especially
so for AtEOXact_GUC(). We can improve matters at the cost
of a bit more space by adding dlist or slist links to the
GUC data structures. This patch invents lists that track
(1) all GUCs with non-default "source";
(2) all GUCs with nonempty state stack (implying they've
been changed in the current transaction);
(3) all GUCs due for reporting to the client.
All of guc.c's performance-critical cases can make use of one or
another of these lists to avoid searching the whole hash table.
In particular, the stack list means that transaction end
doesn't take time proportional to the number of GUCs, but
only to the number changed in the current transaction.
Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us
This gets rid of bsearch() in favor of hashed lookup. The main
advantage is that it becomes far cheaper to add new GUCs, since
we needn't re-sort the pointer array. Adding N new GUCs had
been O(N^2 log N), but now it's closer to O(N). We need to
sort only in SHOW ALL and equivalent functions, which are
hopefully not performance-critical to anybody.
Also, merge GetNumConfigOptions() into get_guc_variables(),
because in a world where the set of GUCs isn't fairly static
you really want to consider those two results as tied together
not independent.
Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us
The only real argument for using malloc directly was that we needed
the ability to not throw error on OOM; but mcxt.c grew that feature
awhile ago.
Keeping the data in a memory context improves accountability and
debuggability --- for example, without this it's almost impossible
to detect memory leaks in the GUC code with anything less costly
than valgrind. Moreover, the next patch in this series will add a
hash table for GUC lookup, and it'd be pretty silly to be using
palloc-dependent hash facilities alongside malloc'd storage of the
underlying data.
This is a bit invasive though, in particular causing an API break
for GUC check hooks that want to modify the GUC's value or use an
"extra" data structure. They must now use guc_malloc() and
guc_free() instead of malloc() and free(). Failure to change
affected code will result in assertion failures or worse; but
thanks to recent effort in the mcxt infrastructure, it shouldn't
be too hard to diagnose such oversights (at least in assert-enabled
builds).
One note is that this changes ParseLongOption() to return short-lived
palloc'd not malloc'd data. There wasn't any caller for which the
previous definition was better.
Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us
We lack a version of repalloc() that supports MCXT_ALLOC_NO_OOM
semantics, so invent repalloc_extended() with the usual set of
flags. repalloc_huge() becomes a legacy wrapper for that.
Also, fix dynahash.c so that it can support HASH_ENTER_NULL
requests when using the default palloc-based allocator.
The only reason it didn't do that already was the lack of the
MCXT_ALLOC_NO_OOM option when that code was written, ages ago.
While here, simplify a few overcomplicated tests in mcxt.c.
Discussion: https://postgr.es/m/2982579.1662416866@sss.pgh.pa.us
Most code prints PIDs as %d, but some code tried to print them as long
or unsigned long. While this is in theory allowed, the fact that PIDs
fit into int is deeply baked into all PostgreSQL code, so these random
deviations don't accomplish anything except confusion.
Note that we still need casts from pid_t to int, because on 64-bit
MinGW, pid_t is long long int. (But per above, actually supporting
that range in PostgreSQL code would be major surgery and probably not
useful.)
Discussion: https://www.postgresql.org/message-id/289c2e45-c7d9-5ce4-7eff-a9e2a33e1580@enterprisedb.com
The comment talked about some Asserts which did not exist and also a
variable name which seems to have long since disappeared.
Rewrite the comment in a way that will hopefully stand the test of
time and inform people why we always write "INSERT 0 <nrows>" instead of
"INSERT <nrows>" in the command completion tag for INSERT.
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/CAApHDvpiUg09AvvGAVopNAKemA9z-kCmt7Fi6HKauc32bKzx4w@mail.gmail.com
In FIPS mode, these calls will fail. By having them in a separate
file, it would make it easier to have an alternative output file or
selectively disable these tests. This isn't done here; this is just
some preparation.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/647f6cc1-473d-f788-ade0-c09201e5ab6a@enterprisedb.com
Commit 3d956d956 allowed the COPY, but it's done by inserting individual
rows to the foreign table, so it can be inefficient due to the overhead
caused by each round-trip to the foreign server. To improve performance
of the COPY in such a case, this patch allows batch insertion, by
extending the multi-insert machinery in CopyFrom() to the foreign-table
case so that we insert multiple rows to the foreign table at once using
the FDW callback routine added by commit b663a4136. This patch also
allows this for postgres_fdw. It is enabled by the "batch_size" option
added by commit b663a4136, which is disabled by default.
When doing batch insertion, we update progress of the COPY command after
performing the FDW callback routine, to count rows not suppressed by the
FDW as well as a BEFORE ROW INSERT trigger. For consistency, this patch
changes the timing of updating it for plain tables: previously, we
updated it immediately after adding each row to the multi-insert buffer,
but we do so only after writing the rows stored in the buffer out to the
table using table_multi_insert(), which I think would be consistent even
with non-batching mode, because in that mode we update it after writing
each row out to the table using table_tuple_insert().
Andrey Lepikhov, heavily revised by me, with review from Ian Barwick,
Andrey Lepikhov, and Zhihong Yu.
Discussion: https://postgr.es/m/bc489202-9855-7550-d64c-ad2d83c24867%40postgrespro.ru
Contrary to what is documented in src/backend/access/transam/README,
ginHeapTupleFastInsert() had a few ordering issues with the way it does
its WAL operations when inserting items in its fast path.
First, when using a separate list, XLogBeginInsert() was being always
called before START_CRIT_SECTION(), and in this case a second thing was
wrong when merging lists, as an exclusive lock was taken on the tail
page *before* calling XLogBeginInsert(). Finally, when inserting items
into a tail page, the order of XLogBeginInsert() and
START_CRIT_SECTION() was reversed. This commit addresses all these
issues by moving the calls of XLogBeginInsert() after all the pages
logged are locked and pinned, within a critical section.
Author: Matthias van de Meent, Zhang Mingli
Discussion: https://postgr.es/m/CAEze2WhL8uLMqynnnCu1LAPwxD5RKEo0nHV+eXGg_N6ELU88HQ@mail.gmail.com
This file needs xlogreader.h only for the XLogReaderState typedef; but
we can dodge that by forward-declaring it. Many files use xlog.h for
reasons other than reading WAL, and it's not good to force all those
files to include xlogreader.h, so take it out.
Surprisingly, there is no fallout in core code from making this change.
Perhaps external code will have to start including xlogreader.h.
An LSN was calculated from a segment number, a segment size and a
position offset, matching exactly the LSN given by the caller of
XLogReaderValidatePageHeader(). This change removes the extra LSN
calculation, relying only on the LSN given by the function caller
instead.
Author: Bharath Rupireddy
Reviewed-by: Richard Guo, Álvaro Herrera, Kyotaro Horiguchi
Discussion: https://postgr.es/m/CALj2ACXuh4Ms9j9sxMYdtHEe=5sFcyrs-GAHyADu_A_G71kZTg@mail.gmail.com
The postmaster is not supposed to do anything that depends
fundamentally on shared memory contents, because that creates
the risk that a backend crash that trashes shared memory will
take the postmaster down with it, preventing automatic recovery.
In commit 969d7cd43 I lost sight of this principle and coded
AssignPostmasterChildSlot() in such a way that it could fail
or even crash if the shared PMSignalState structure became
corrupted. Remarkably, we've not seen field reports of such
crashes; but I managed to induce one while testing the recent
changes around palloc chunk headers.
To fix, make a semi-duplicative state array inside the postmaster
so that we need consult only local state while choosing a "child
slot" for a new backend. Ensure that other postmaster-executed
routines in pmsignal.c don't have critical dependencies on the
shared state, either. Corruption of PMSignalState might now
lead ReleasePostmasterChildSlot() to conclude that backend X
failed, when actually backend Y was the one that trashed things.
But that doesn't matter, because we'll force a cluster-wide reset
regardless.
Back-patch to all supported branches, since this is an old bug.
Discussion: https://postgr.es/m/3436789.1665187055@sss.pgh.pa.us
DEFAULT markers appearing in an INSERT on an updatable view
could be mis-processed if they were in a multi-row VALUES clause.
This would lead to strange errors such as "cache lookup failed
for type NNNN", or in older branches even to crashes.
The cause is that commit 41531e42d tried to re-use rewriteValuesRTE()
to remove any SetToDefault nodes (that hadn't previously been replaced
by the view's own default values) appearing in "product" queries,
that is DO ALSO queries. That's fundamentally wrong because the
DO ALSO queries might not even be INSERTs; and even if they are,
their targetlists don't necessarily match the view's column list,
so that almost all the logic in rewriteValuesRTE() is inapplicable.
What we want is a narrow focus on replacing any such nodes with NULL
constants. (That is, in this context we are interpreting the defaults
as being strictly those of the view itself; and we already replaced
any that aren't NULL.) We could add still more !force_nulls tests
to further lobotomize rewriteValuesRTE(); but it seems cleaner to
split out this case to a new function, restoring rewriteValuesRTE()
to the charter it had before.
Per bug #17633 from jiye_sw. Patch by me, but thanks to
Richard Guo and Japin Li for initial investigation.
Back-patch to all supported branches, as the previous fix was.
Discussion: https://postgr.es/m/17633-98cc85e1fa91e905@postgresql.org
Make a common replication origin name formatting function to replace
multiple snprintf() expressions. This also includes logic previously done
by ReplicationOriginNameForTablesync().
This makes the code to generate the origin name consistent among apply
worker and tablesync worker.
Author: Peter Smith
Reviewed-By: Aleksander Alekseev
Discussion: https://postgr.es/m/CAHut%2BPsa8hhfSE6ozUK-ih7GkQziAVAf4f3bqiXEj2nQiu-43g%40mail.gmail.com
This commit expands the coverage of pg_hba.conf with checks specific to
role memberships (one "root" role combined with a member and a
non-member). Coverage is added for the database keywords "samegroup"
and "samerole", where the specified role has to be be a member of the
role with the same name as the requested database, and '+' on the user
entry, where members are allowed. These tests are plugged in the
authentication test 001_password.pl as of extra connection attempts
combined with resets of pg_hba.conf, making them rather cheap.
Author: Nathan Bossart
Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/20221009211348.GB900071@nathanxps13
This is useful as a way for extensions to process COPY TO rows in the
way they see fit (say auditing, analytics, backend, etc.) without the
need to invoke an external process running as the OS user running the
backend through PROGRAM that requires superuser rights. COPY FROM
already provides a similar callback for logical replication. For COPY
TO, the callback is triggered when we are ready to send a row in
CopySendEndOfRow(), which is the same code path as when sending a row
to a frontend or a pipe/file.
A small test module, test_copy_callbacks, is added to provide some
coverage for this facility.
Author: Bilva Sanaba, Nathan Bossart
Discussion: https://postgr.es/m/253C21D1-FCEB-41D9-A2AF-E6517015B7D7@amazon.com
Before commit c6e0fe1f2, functions such as AllocSetFree could pretty
safely presume that they were given a valid chunk pointer for their
own type of context, because the indirect call through a memory
context object and method struct would be very unlikely to work
otherwise. But now, if pfree() is mistakenly invoked on a pointer
to garbage, we have three chances in eight of ending up at one of
these functions. That means we need to take extra measures to
verify that we are looking at what we're supposed to be looking at,
especially in debug builds.
Hence, add code to verify that the chunk's back-link to a block header
leads to a memory context object that satisfies the right sort of
IsA() check. This is still a bit weaker than what we did before,
but for the moment assume that an IsA() check is sufficient.
As a compromise between speed and safety, implement these checks
as Asserts when dealing with small chunks but plain test-and-elogs
when dealing with large (external) chunks. The latter case should
not be too performance-critical, but the former case probably is.
In slab.c, all chunks are small; but nonetheless use a plain test
in SlabRealloc, because that is certainly not performance-critical,
indeed we should be suspicious that it's being called in error.
In aset.c, additionally add some assertions that the "value" field
of the chunk header is within the small range allowed for freelist
indexes. Without that, we might find ourselves trying to wipe
most of memory when CLOBBER_FREED_MEMORY is enabled, or scribbling
on a "freelist header" that's far away from the context object.
Eventually, field experience might show us that it's smarter for
these tests to be active always, but for now we'll try to get
away with just having them as assertions.
While at it, also be more uniform about asserting that context
objects passed as parameters are of the type we expect. Some
places missed that altogether, and slab.c was for no very good
reason doing it differently from the other allocators.
Discussion: https://postgr.es/m/3578387.1665244345@sss.pgh.pa.us
Remove the Trap and TrapMacro macros, which were nearly unused
and confusingly had the opposite condition polarity from the
otherwise-functionally-equivalent Assert macros.
Having done that, it's very hard to justify carrying the errorType
argument of ExceptionalCondition, so drop that too, and just
let it assume everything's an Assert. This saves about 64K
of code space as of current HEAD.
Discussion: https://postgr.es/m/3928703.1665345117@sss.pgh.pa.us
Instead of Abs() for int64, use the C standard functions labs() or
llabs() as appropriate. Define a small wrapper around them that
matches our definition of int64. (labs() is C90, llabs() is C99.)
Reviewed-by: Zhang Mingli <zmlpostgres@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4beb42b5-216b-bce8-d452-d924d5794c63%40enterprisedb.com
Previously PgStat_StatReplSlotEntry contained the slotname, which was mainly
used when writing out the stats during shutdown, to identify the slot in the
serialized data (at runtime the index in ReplicationSlotCtl->replication_slots
is used, but that can change during a restart). Unfortunately the slotname was
overwritten when the slot's stats were reset.
That turned out to only cause "real" problems if the slot was active during
the reset, triggering an assertion failure at the next
pgstat_report_replslot(). In other paths the stats were re-initialized during
pgstat_acquire_replslot().
Fix this by removing slotname from PgStat_StatReplSlotEntry. Instead we can
get the slot's name from the slot itself. Besides fixing a bug, this also is
architecturally cleaner (a name is not really statistics). This is safe
because stats, for a slot removed while shut down, will not be restored at
startup.
In 15 the slotname is not removed, but renamed, to avoid changing the stats
format. In master, bump PGSTAT_FILE_FORMAT_ID.
This commit does not contain a test for the fix. I think this can only be
tested by a tap test starting pg_recvlogical in the background and checking
pg_recvlogical's output. That type of test is notoriously hard to be reliable,
so committing it shortly before the release is wrapped seems like a bad idea.
Reported-by: Jaime Casanova <jcasanov@systemguards.com.ec>
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/YxfagaTXUNa9ggLb@ahch-to
Backpatch: 15-, where the bug was introduced in 5891c7a8ed
This way we don't need RANLIB anymore, making it a bit simpler for the meson
build to generate Makefile.global for PGXS compatibility.
FreeBSD, NetBSD, OpenBSD, the only platforms where we didn't use AROPT=crs,
all have supported the 's' option for a long time.
On macOS we ran ranlib after installing a static library. This was added a
long time ago, in 58ad65ec2d. I cannot reproduce an issue in more recent
macOS versions. This is removed now.
Based on discussion with Tom, I left the 'touch' at the end of static
libraries generation, added in 826eff57c4, in place. While it looks like
current versions of Apple's ar/ranlib don't need it, it was needed not too
long ago.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20221005200710.luvw5evhwf6clig6@awork3.anarazel.de
There are a number of bugs in this area. Two of them are fixed here,
namely:
1. get_relation_idx_constraint_oid does not restrict the type of
constraint that's returned, so with sufficient bad luck it can
return the OID of a foreign key constraint. This has the effect that
a primary key in a partition can end up as a child of a foreign key,
which makes no sense (it needs to be the child of the equivalent
primary key.)
Change the API contract so that only index-backed constraints are
returned, mimicking get_constraint_index().
2. Both CloneFkReferenced and CloneFkReferencing clone a
self-referencing foreign key, so the partition ends up with
a duplicate foreign key. Change the former function to ignore such
constraints.
Add some tests to verify that things are better now. (However, these
new tests show some additional misbehavior that will be fixed later --
namely that there's a constraint marked NOT VALID.)
Backpatch to 12, where these constraints are possible at all.
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Discussion: https://postgr.es/m/20220603154232.1715b14c@karst
Commit c6e0fe1f2 was a shade too trusting that any pointer passed
to pfree, repalloc, etc will point at a valid chunk. Notably,
passing a pointer that was actually obtained from malloc tended
to result in obscure assertion failures, if not worse. (On FreeBSD
I've seen such mistakes take down the entire cluster, seemingly as
a result of clobbering shared memory.)
To improve matters, extend the mcxt_methods[] array so that it
has entries for every possible MemoryContextMethodID bit-pattern,
with the currently unassigned ID codes pointing to error-reporting
functions. Then, fiddle with the ID assignments so that patterns
likely to be associated with bad pointers aren't valid ID codes.
In particular, we should avoid assigning bit patterns 000 (zeroed
memory) and 111 (wipe_mem'd memory).
It turns out that on glibc (Linux), malloc uses chunk headers that
have flag bits in the same place we keep MemoryContextMethodID,
and that the bit patterns 000, 001, 010 are the only ones we'll
see as long as the backend isn't threaded. So we can have very
robust detection of pfree'ing a malloc-assigned block on that
platform, at least so long as we can refrain from using up those
ID codes. On other platforms, we don't have such a good guarantee,
but keeping 000 reserved will be enough to catch many such cases.
While here, make GetMemoryChunkMethodID() local to mcxt.c, as there
seems no need for it to be exposed even in memutils_internal.h.
Patch by me, with suggestions from Andres Freund and David Rowley.
Discussion: https://postgr.es/m/2910981.1665080361@sss.pgh.pa.us
This substantially speeds up building for windows, due to the vast amount of
headers included via windows.h. A cross build from linux targetting mingw goes
from
994.11user 136.43system 0:31.58elapsed 3579%CPU
to
422.41user 89.05system 0:14.35elapsed 3562%CPU
The wins on windows are similar-ish (but I don't have a system at hand just
now for actual numbers). Targetting other operating systems the wins are far
smaller (tested linux, macOS, FreeBSD).
For now precompiled headers are disabled by default, it's not clear how well
they work on all platforms. E.g. on FreeBSD gcc doesn't seem to have working
support, but clang does.
When doing a full build precompiled headers are only beneficial for targets
with multiple .c files, as meson builds a separate precompiled header for each
target (so that different compilation options take effect). This commit
therefore only changes target with at least two .c files to use precompiled
headers.
Because this commit adds b_pch=false to the default_options new build
directories will have precompiled headers disabled by default, however
existing build directories will continue use the default value of b_pch, which
is true.
Note that using precompiled headers with ccache requires setting
CCACHE_SLOPPINESS=pch_defines,time_macros to get hits.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CA+hUKG+50eOUbN++ocDc0Qnp9Pvmou23DSXu=ZA6fepOcftKqA@mail.gmail.com
Discussion: https://postgr.es/m/c5736f70-bb6d-8d25-e35c-e3d886e4e905@enterprisedb.com
Discussion: https://postgr.es/m/20190826054000.GE7005%40paquier.xyz
Previously, the subscription stats entry was created when the first
stats, i.e., an error on apply worker or tablesync worker, were
reported. Therefore, the stats_reset field was not updated by
pg_stat_reset_subscription_stats() if the stats entry was not
populated yet, which was different behavior than other statistics.
This change creates the subscription stats entry and initializes it at
CREATE SUBSCRIPTION time.
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Author: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAAKRu_Zqd-e5imT_3-ZiQv1cfsWuy16OJTiUaCvqpq4V7GVdSg@mail.gmail.com
When using precompiled headers, we cannot pre-define macros for the system
headers from within .c files, as headers are already processed before
the #define in the C file is reached. But we can pre-define using
-DFD_SETSIZE, as long as that's also used when building the precompiled header.
A few files #define FD_SETSIZE 1024 on windows, as the default is only 64. I
am hesitant to change FD_SETSIZE globally on windows, due to
src/backend/port/win32/socket.c using it to size on-stack arrays. Instead add
-DFD_SETSIZE=1024 when building the specific targets needing it.
We likely should move away from using select() in those places, but that's a
larger change.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20221005190829.lda7ttalh4mzrvf4@awork3.anarazel.de
Discussion: https://postgr.es/m/CA+hUKG+50eOUbN++ocDc0Qnp9Pvmou23DSXu=ZA6fepOcftKqA@mail.gmail.com
Discussion: https://postgr.es/m/20190826054000.GE7005%40paquier.xyz
MemoryContextContains is no longer reliable in the wake of c6e0fe1f2,
because there's no longer very much redundancy in chunk headers.
(It wasn't *completely* reliable even before that, as there was a
chance of a false positive if you passed it something that didn't
point to an mcxt chunk at all. But it was generally good enough.)
Hence, remove it. There is no remaining core code that requires it.
Extensions that have been using it might be able to substitute a
test like "GetMemoryChunkContext(ptr) == context", recognizing that
this explicitly requires that the pointer point to some chunk.
Tom Lane and David Rowley
Discussion: https://postgr.es/m/1913788.1664898906@sss.pgh.pa.us
MemoryContextContains is no longer reliable in the wake of c6e0fe1f2,
so we need to get rid of these uses.
It appears that there's no really good reason to force the result of
an aggregate's finalfn or serialfn to be allocated in the per-tuple
context. The only other plausible case is that the result points to
or into the aggregate's transition value, and that's fine because it
will last as long as we need it to. (This conclusion depends on the
assumption that finalfns are not allowed to scribble on the transition
value, but we've long required that.) So we can just drop the
MemoryContextContains plus datumCopy business, although we do need
to take care to not return a read-write pointer when the transition
value is an expanded datum.
Likewise, we don't really need to force the result of a window
function to be in the output context. In this case, the plausible
alternative is that it's pointing into the temporary tuple slot used
by WinGetFuncArgInPartition or WinGetFuncArgInFrame (since those
functions could return such a pointer, which might become the window
function's result). That will hold still for long enough, unless
there is another window function using the same WindowObject.
I'm content to always perform a datumCopy when there's more than one
such function.
On net, these changes should provide small speed improvements as well
as removing problematic code.
Tom Lane and David Rowley
Discussion: https://postgr.es/m/1913788.1664898906@sss.pgh.pa.us
The RecoveryLockLists data structure, which tracks all exclusive
locks that the startup process is holding on behalf of transactions
being replayed, did not have any provision for avoiding duplicate
entries for the same lock. Maybe that was okay when the code was
first written. However, modern practice is for checkpoints to
write fresh lists of all active exclusive locks into the WAL.
Thus, an exclusive lock that survives across multiple checkpoints
causes bloat in standbys' startup processes. If there are a lot
of such locks this can look like a memory leak, and it's even
possible to drive the startup process into a palloc failure from
an over-length List.
To fix, use a hash table instead of simple lists to track the
locks being held. Allowing for dynahash overhead, this requires
a little more space per lock than the old way (although it's the
same size as what we were allocating prior to c6e0fe1f2). It's
probably a shade slower too. However, testing indicates that the
penalty is negligible on ordinary workloads, so let's make this
change to improve robustness in extreme cases.
Patch by me, per report from Dmitriy Kuzmin. No back-patch
(for now anyway), since it seems that a significant improvement
would only occur in corner cases.
Discussion: https://postgr.es/m/CAHLDt=_ts0A7Agn=hCpUh+RCFkxd+G6uuT=kcTfqFtGur0dp=A@mail.gmail.com
ts_locale.c omitted support for "isalnum" tests, perhaps on the
grounds that there were initially no use-cases for that. However,
both ltree and pg_trgm need such tests, and we do also have one
use-case now in the core backend. The workaround of testing
isalpha and isdigit separately seems quite inefficient, especially
when dealing with multibyte characters; so let's fill in the
missing support.
Discussion: https://postgr.es/m/2548310.1664999615@sss.pgh.pa.us
The test is changed to test for connection strings rather than specific
roles, and the reset logic of pg_hba.conf is extended so as the database
and user name entries can be directly specified. This is aimed at being
used as a base for more test scenarios of pg_hba.conf and authentication
paths.
Author: Bertrand Drouvot, Michael Paquier
Discussion: https://postgr.es/m/Yz0xO0emJ+mxtj2a@paquier.xyz
This optional parameter can be specified in cases where there are nested
PG_TRY() statements within a function in order to stop the compiler from
issuing warnings about shadowed local variables when compiling with
-Wshadow. The optional parameter is used as a suffix on the variable
names declared within the PG_TRY(), PG_CATCH(), PG_FINALLY() and
PG_END_TRY() macros. The parameter, if specified, must be the same in
each component macro of the given PG_TRY() block.
This also adjusts the single case where we have nested PG_TRY() statements
to add a parameter to the inner-most PG_TRY().
This reduces the number of compiler warnings when compiling with
-Wshadow=compatible-local from 5 down to 1.
Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvqWGMdB_pATeUqE=JCtNqNxObPOJ00jFEa2_sZ20j_Wvg@mail.gmail.com
The generated resource files aren't exactly the same ones as the old
buildsystems generate. Previously "InternalName" and "OriginalFileName" were
mostly wrong / not set (despite being required), but that was hard to fix in
at least the make build. Additionally, the meson build falls back to a
"auto-generated" description when not set, and doesn't set it in a few cases -
unlikely that anybody looks at these descriptions in detail.
Author: Andres Freund <andres@anarazel.de>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Required for correct resource file generation, as the resource files should
only be added to the shared library.
This also fixes a bunch of issues in the .pc files.
Previously I tried to avoid building sources twice, once for the static and
once for the shared libraries. We could still do so, but it's not clear that
it's worth the complication.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/20220927011951.j3h4o7n6bhf7dwau@awork3.anarazel.de
Improvements:
- we don't need -DFRONTEND for libpq anymore since 1d77afefbd
- the .pc file contents for a static libpq were wrong (referencing
{pgport, common}_shlib)
- incidentally fixes meson not supporting link_whole on AIX yet
- added explanatory comments
Previously I tried to avoid building libpq's sources twice, once for the
static and once for the shared library. We could still do so, but it's not
clear that it's worth the complication.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In a similar effort to f01592f91, here we mostly rename shadowed local
variables to remove the warnings produced when compiling with
-Wshadow=compatible-local.
This fixes 63 warnings and leaves just 5.
Author: Justin Pryzby, David Rowley
Reviewed-by: Justin Pryzby
Discussion https://postgr.es/m/20220817145434.GC26426%40telsasoft.com
When scanning for the end of WAL, pg_resetwal has been maintaining its
own internal logic to calculate segment numbers and to parse a WAL
segment name for its timeline and segment number. The code claimed for
example that XLogFromFileName() cannot be used because it lacks the
possibility of specifying a WAL segment size, which is not the case
since fc49e24, that has made the WAL segment size configurable at
initialization time, extending this routine to do so.
Similarly, this switches one segment number calculation to use
XLByteToSeg() rather than the same logic present in xlog_internal.h.
While on it, switch to TimeLineID in pg_resetwal.c for TLI numbers
parsed from segment names, to be more consistent with
XLogFromFileName(). The maths are exactly the same, but the code gets
simplified.
Author: Bharath Rupireddy
Reviewed-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/CALj2ACX+E_jnwqH_jmjhNG8BczJTNRTOLpw8K1CB1OcB48MJ8w@mail.gmail.com
This improves the tab completion of psql on a few points:
- Provide a list of subscriptions on \dRs.
- Provide a list of publications on \dRp.
- Add CURRENT_ROLE, CURRENT_USER, SESSION_USER when OWNER TO is provided
at the end of a query (as defined by RoleSpec in gram.y).
Author: Vignesh C
Reviewed-by: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/CALDaNm3toRBt6c6saY3174f7CsGztXRvVvfWbikkJEXY7x5WAA@mail.gmail.com
This cleans up a couple of areas:
- Remove XLogSegNo calculation for the last WAL segment in backup in
xlog.c (7d70809 has moved this logic entirely to xlogbackup.c when
building the contents of the backup history file).
- Remove check on log_min_duration in analyze.c, as it is already true
where this code path is reached.
- Simplify call to find_option() in guc.c.
Author: Ranier Vilela
Reviewed-by: Masahiko Sawada
Discussion: https://postgr.es/m/CAEudQArCDQQiPiFR16=yu9k5s2tp4tgEe1U1ZbkW4ofx81AWWQ@mail.gmail.com
This commit expands the TAP tests of pg_upgrade when running these with
different major versions for the "old" cluster (to-be-upgraded) and the
"new" cluster (upgraded-to), by backporting some of the buildfarm
facilities directory into the script:
- Remove comments from the dump files, avoiding version-dependent
information.
- Remove empty lines from the dump files.
- Use --extra-float-digits=0 in the pg_dump command, when using an "old"
cluster with version equal to or lower than v11.
- Use --wal-segsize and --allow-group-access in initdb only when the
"old" cluster is equal to or higher than v11.
This allows the tests to pass down to v14 with the main regression test
suite, while v9.5~13 still generate some diffs, but these are minimal
compared to what happened before this commit. Much more could be done,
especially around dump differences with function and procedures (these
can also be avoided with direct manipulation of the dumps loaded, for
example, in a way similar to the buildfarm), but at least the basics are
in place now.
Reviewed-by: Justin Pryzby, Anton A. Melnikov
Discussion: https://postgr.es/m/Yox1ME99GhAemMq1@paquier.xyz
Otherwise we don't use LLVM's flags when building llvmjit_wrap.cpp and
llvmjit_inline.cpp. That can cause compile time failures if the C++ compiler
doesn't default to a new enough C++ standards version and link time failures
due to ABI influencing flags like -fno-rtti.
The pre-v15 behavior was to discard all but the last result,
but with the new behavior of printing all results by default,
we will send each such result to the \g file. However,
we're still opening and closing the \g file for each result,
so you lose all but the last result anyway. Move the output-file
state up to ExecQueryAndProcessResults so that we open/close the
\g file only once per command string.
To support this without changing other behavior, we must
adjust PrintQueryResult to have separate FILE * arguments
for query and status output (since status output has never
gone to the \g file). That in turn makes it a good idea
to push the responsibility for fflush'ing output down to
PrintQueryTuples and PrintQueryStatus.
Also fix an infinite loop if COPY IN/OUT is attempted in \watch.
We used to reject that, but that error exit path got broken
somewhere along the line in v15. There seems no real reason
to reject it anyway as the code now stands, so just remove
the error exit and make sure that COPY OUT data goes to the
right place.
Also remove PrintQueryResult's unused is_watch parameter,
and make some other cosmetic cleanups (adjust obsolete
comments, break some overly-long lines).
Daniel Vérité and Tom Lane
Discussion: https://postgr.es/m/4333844c-2244-4d6e-a49a-1d483fbe304f@manitou-mail.org
This reverts commit db0d67db24 and
several follow-on fixes. The idea of making a cost-based choice
of the order of the sorting columns is not fundamentally unsound,
but it requires cost information and data statistics that we don't
really have. For example, relying on procost to distinguish the
relative costs of different sort comparators is pretty pointless
so long as most such comparator functions are labeled with cost 1.0.
Moreover, estimating the number of comparisons done by Quicksort
requires more than just an estimate of the number of distinct values
in the input: you also need some idea of the sizes of the larger
groups, if you want an estimate that's good to better than a factor of
three or so. That's data that's often unknown or not very reliable.
Worse, to arrive at estimates of the number of calls made to the
lower-order-column comparison functions, the code needs to make
estimates of the numbers of distinct values of multiple columns,
which are necessarily even less trustworthy than per-column stats.
Even if all the inputs are perfectly reliable, the cost algorithm
as-implemented cannot offer useful information about how to order
sorting columns beyond the point at which the average group size
is estimated to drop to 1.
Close inspection of the code added by db0d67db2 shows that there
are also multiple small bugs. These could have been fixed, but
there's not much point if we don't trust the estimates to be
accurate in-principle.
Finally, the changes in cost_sort's behavior made for very large
changes (often a factor of 2 or so) in the cost estimates for all
sorting operations, not only those for multi-column GROUP BY.
That naturally changes plan choices in many situations, and there's
precious little evidence to show that the changes are for the better.
Given the above doubts about whether the new estimates are really
trustworthy, it's hard to summon much confidence that these changes
are better on the average.
Since we're hard up against the release deadline for v15, let's
revert these changes for now. We can always try again later.
Note: in v15, I left T_PathKeyInfo in place in nodes.h even though
it's unreferenced. Removing it would be an ABI break, and it seems
a bit late in the release cycle for that.
Discussion: https://postgr.es/m/TYAPR01MB586665EB5FB2C3807E893941F5579@TYAPR01MB5866.jpnprd01.prod.outlook.com
This commit introduces an authentication test for the peer method, as of
a set of scenarios with and without a user name map. The script is
automatically skipped if peer is not supported in the environment where
this test is run, checking this behavior by attempting a connection
first on a cluster up and running.
Author: Bertrand Drouvot
Discussion: https://postgr.es/m/aa60994b-1c66-ca7a-dab9-9a200dbac3d2@amazon.com
Both check_application_name() and check_cluster_name() use
pg_clean_ascii() but didn't release the memory. Depending on when the
GUC is set, this might be cleaned up at some later time or it would
leak postmaster memory once. In any case, it seems better not to have
to rely on such analysis and make the code locally robust. Also, this
makes Valgrind happier.
Author: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Jacob Champion <jchampion@timescale.com>
Discussion: https://www.postgresql.org/message-id/CAD21AoBmFNy9MPfA0UUbMubQqH3AaK5U3mrv6pSeWrwCk3LJ8g@mail.gmail.com
Commit 34f581c39 intended to ensure that RelationGetBufferForTuple
would acquire a visibility-map page pin in case the otherBuffer's
all-visible bit had become set since we last had lock on that page.
But I missed a case: when we're extending the relation, VM concerns
were dealt with only in the relatively-less-likely case that we
fail to conditionally lock the otherBuffer. I think I'd believed
that we couldn't need to worry about it if the conditional lock
succeeds, which is true for the target buffer; but the otherBuffer
was unlocked for awhile so its bit might be set anyway. So we need
to do the GetVisibilityMapPins dance, and then also recheck the
page's free space, in both cases.
Per report from Jaime Casanova. Back-patch to v12 as the previous
patch was (although there's still no evidence that the bug is
reachable pre-v14).
Discussion: https://postgr.es/m/E1lWLjP-00006Y-Ml@gemulon.postgresql.org
Currently, PQsslAttributeNames() returns the same list of attribute
names regardless of its conn parameter. This patch changes it to
have behavior parallel to what 80a05679d installed for PQsslAttribute:
you get OpenSSL's attributes if conn is NULL or is an SSL-encrypted
connection, or an empty list if conn is a non-encrypted connection.
The point of this is to have sensible connection-dependent behavior
in case we ever support multiple SSL libraries. The behavior for
NULL can be defined as "the attributes for the default SSL library",
parallel to what PQsslAttribute(NULL, "library") does.
Since this is mostly just future-proofing, no back-patch.
Discussion: https://postgr.es/m/17625-fc47c78b7d71b534@postgresql.org
I (Álvaro) broke tab-completion for GRANT .. ALL TABLES IN SCHEMA while
removing ALL from the publication syntax for schemas in the
aforementioned commit. I also missed to update a bunch of
tab-completion rules for ALTER/CREATE PUBLICATION that match each
individual piece of ALL TABLES IN SCHEMA. Repair those bugs.
While fixing up that commit, update a couple of outdated comments
related to the same change.
Backpatch to 15.
Author: Shi yu <shiy.fnst@fujitsu.com>
Reviewed-by: Peter Smith <smithpb2250@gmail.com>
Discussion: https://postgr.es/m/OSZPR01MB6310FCE8609185A56344EED2FD559@OSZPR01MB6310.jpnprd01.prod.outlook.com
The last caller of UnpinBuffer() that did not want to adjust
CurrentResourceOwner was removed in 2d115e4, and nothing has been
introduced in bufmgr.c to do the same thing since. This simplifies 10
code paths.
Author: Aleksander Alekseev
Reviewed-by: Nathan Bossart, Zhang Mingli, Bharath Rupireddy
Discussion: https://postgr.es/m/CAJ7c6TOmmFpb6ohurLhTC7hKNJWGzdwf8s4EAtAZxD48g-e6Jw@mail.gmail.com
Commit ebc8b7d44 intended to change the behavior of
PQsslAttribute(NULL, "library"), but accidentally also changed
what happens with a non-NULL conn pointer. Undo that so that
only the intended behavior change happens. Clarify some
associated documentation.
Per bug #17625 from Heath Lord. Back-patch to v15.
Discussion: https://postgr.es/m/17625-fc47c78b7d71b534@postgresql.org
The one about "terminating process to release replication slot" told
you nothing about why that was happening. The one about "invalidating
slot because its restart_lsn exceeds max_slot_wal_keep_size" told you
what was happening, but violated our message style guideline about
keeping the primary message short. Add DETAIL/HINT lines to carry
the appropriate detail and make the two cases more uniform.
While here, fix bogus test logic in 019_replslot_limit.pl: if it timed
out without seeing the expected log message, no test failure would be
reported. This is flat broken since commit 549ec201d removed the test
counts; even before that it was horribly bad style, since you'd only
get told that not all tests had been run.
Kyotaro Horiguchi, reviewed by Bertrand Drouvot; test fixes by me
Discussion: https://postgr.es/m/20211214.130456.2233153190058148084.horikyota.ntt@gmail.com
Up to now, the ID values returned by pg_stat_get_backend_idset() and
used by pg_stat_get_backend_activity() and allied functions were just
indexes into a local array of sessions seen by the last stats refresh.
This is problematic for a few reasons. The "ID" of a session can vary
over its existence, which is surprising. Also, while these numbers
often match the "backend ID" used for purposes like temp schema
assignment, that isn't reliably true. We can fairly cheaply switch
things around to make these numbers actually be the sessions' backend
IDs. The added test case illustrates that with this definition, the
temp schema used by a given session can be obtained given its PID.
While here, delete some dead code that guarded against getting
a NULL return from pgstat_fetch_stat_local_beentry(). That can't
happen as long as the caller is careful to pass an in-range array
index, as all the callers are. (This code may not have been dead
when written, but it surely is now.)
Nathan Bossart
Discussion: https://postgr.es/m/20220815205811.GA250990@nathanxps13
SYSTEM_USER is a reserved keyword of the SQL specification that,
roughly described, is aimed at reporting some information about the
system user who has connected to the database server. It may include
implementation-specific information about the means by the user
connected, like an authentication method.
This commit implements SYSTEM_USER as of auth_method:identity, where
"auth_method" is a keyword about the authentication method used to log
into the server (like peer, md5, scram-sha-256, gss, etc.) and
"identity" is the authentication identity as introduced by 9afffcb (peer
sets authn to the OS user name, gss to the user principal, etc.). This
format has been suggested by Tom Lane.
Note that thanks to d951052, SYSTEM_USER is available to parallel
workers.
Bump catalog version.
Author: Bertrand Drouvot
Reviewed-by: Jacob Champion, Joe Conway, Álvaro Herrera, Michael Paquier
Discussion: https://postgr.es/m/7e692b8c-0b11-45db-1cad-3afc5b57409f@amazon.com
We'd like to use precompiled headers on windows to reduce compile times. Right
now we rely on defining UMDF_USING_NTSTATUS before including postgres.h in a few
select places - which doesn't work with precompiled headers. Instead define
it globally.
When UMDF_USING_NTSTATUS is defined we need to explicitly include ntstatus.h,
winternl.h to get a comparable set of symbols. Right now these includes would
be required in a number of non-platform-specific .c files - to avoid that,
include them in win32_port.h. Based on my measurements that doesn't increase
compile times measurably.
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/20220927011951.j3h4o7n6bhf7dwau@awork3.anarazel.de
This doesn't end up with a triple that's exactly the same as config.guess -
it'd be hard to achieve that and it doesn't seem required. We can't rely on
config.guess as we don't necessarily have a /bin/sh on windows, e.g., when
building on windows with msvc.
This isn't perfect, e.g., clang works on windows as well. But I suspect we'd
need a bunch of other changes to make clang on windows work, and we haven't
supported it historically.
Discussion: http://postgr.es/m/20220928022724.erzuk5v4ai4b53do@awork3.anarazel.de
Commits cf112c12 and a0dc8271 were a little too hasty in getting rid of
the pg_ prefixes where we use pread(), pwrite() and vectored variants.
We dropped support for ancient Unixes where we needed to use lseek() to
implement replacements for those, but it turns out that Windows also
changes the current position even when you pass in an offset to
ReadFile() and WriteFile() if the file handle is synchronous, despite
its documentation saying otherwise.
Switching to asynchronous file handles would fix that, but have other
complications. For now let's just put back the pg_ prefix and add some
comments to highlight the non-standard side-effect, which we can now
describe as Windows-only.
Reported-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/20220923202439.GA1156054%40nathanxps13
91e9e89dc modified nodeSort.c so that it used datum sorts when the
targetlist of the outer node contained only a single column. That commit
failed to recognise that the Datum returned by tuplesort_getdatum() must
be pfree'd when the type is a byref type. Ronan Dunklau did originally
propose the patch with that restriction, but that, probably through my own
fault, got lost during further development work.
Due to the timing of this report (PG15 RC1 is almost out the door), let's
just restrict the datum sort optimization to apply for byval types only.
We might want to look harder into making this work for byref types in
PG16.
Reported-by: Önder Kalacı
Diagnosis-by: Tom Lane
Discussion: https://postgr.es/m/CACawEhVxe0ufR26UcqtU7GYGRuubq3p6ZWPGXL4cxy_uexpAAQ@mail.gmail.com
Backpatch-through: 15, where 91e9e89dc was introduced.
Fetch the next-item pointer before the call not after, so that
we aren't dereferencing a dangling pointer if the callback
deregistered itself during the call. The risky coding pattern
appears in CallXactCallbacks, CallSubXactCallbacks, and
ResourceOwnerReleaseInternal. (There are some other places that
might be at hazard if they offered deregistration functionality,
but they don't.)
I (tgl) considered back-patching this, but desisted because it
wouldn't be very safe for extensions to rely on this working in
pre-v16 branches.
Hao Wu
Discussion: https://postgr.es/m/CAH+9SWXTiERkmhRke+QCcc+jRH8d5fFHTxh8ZK0-Yn4BSpyaAg@mail.gmail.com
This prevents marking the argument string for translation for gettext,
and it also prevents the given string (which is already translated) from
being translated at runtime.
Also, mark the strings used as arguments to check_rolespec_name for
translation.
Backpatch all the way back as appropriate. None of this is caught by
any tests (necessarily so), so I verified it manually.
There are still some alignment-related failures in the buildfarm,
which might or might not be able to be fixed quickly, but I've also
just realized that it increased the size of many WAL records by 4 bytes
because a block reference contains a RelFileLocator. The effect of that
hasn't been studied or discussed, so revert for now.
SharedInvalSmgrMsg can't require 8-byte alignment, because then
SharedInvalidationMessage will require 8-byte alignment, which will
then cause ParseCommitRecord to fail on machines that are picky
about alignment, because it assumes that everything that gets
packed into a commit record requires only 4-byte alignment.
Another problem with 05d4cbf9b6.
Discussion: http://postgr.es/m/3825454.1664310917@sss.pgh.pa.us
The previous macro implementations just cast the argument to a target
type but did not check whether the input type was appropriate. The
function implementation can do better type checking of the input type.
For the *GetDatumFast() macros, converting to an inline function
doesn't work in the !USE_FLOAT8_BYVAL case, but we can use
AssertVariableIsOfTypeMacro() to get a similar level of type checking.
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/8528fb7e-0aa2-6b54-85fb-0c0886dbd6ed%40enterprisedb.com
RelFileNumbers are now assigned using a separate counter, instead of
being assigned from the OID counter. This counter never wraps around:
if all 2^56 possible RelFileNumbers are used, an internal error
occurs. As the cluster is limited to 2^64 total bytes of WAL, this
limitation should not cause a problem in practice.
If the counter were 64 bits wide rather than 56 bits wide, we would
need to increase the width of the BufferTag, which might adversely
impact buffer lookup performance. Also, this lets us use bigint for
pg_class.relfilenode and other places where these values are exposed
at the SQL level without worrying about overflow.
This should remove the need to keep "tombstone" files around until
the next checkpoint when relations are removed. We do that to keep
RelFileNumbers from being recycled, but now that won't happen
anyway. However, this patch doesn't actually change anything in
this area; it just makes it possible for a future patch to do so.
Dilip Kumar, based on an idea from Andres Freund, who also reviewed
some earlier versions of the patch. Further review and some
wordsmithing by me. Also reviewed at various points by Ashutosh
Sharma, Vignesh C, Amul Sul, Álvaro Herrera, and Tom Lane.
Discussion: http://postgr.es/m/CA+Tgmobp7+7kmi4gkq7Y+4AM9fTvL+O1oQ4-5gFTT+6Ng-dQ=g@mail.gmail.com
Previously, these were declared in postgres_ext.h, but they are not
needed nearly so widely as the OID declarations, so that doesn't
necessarily make sense. Also, because postgres_ext.h is included
before most of c.h has been processed, the previous location creates
some problems for a pending patch.
Patch by me, reviewed by Dilip Kumar.
Discussion: http://postgr.es/m/CA+TgmoYc8oevMqRokZQ4y_6aRn-7XQny1JBr5DyWR_jiFtONHw@mail.gmail.com
Push the units fields over to the left so that all the single-bit
flags can be together. I considered rearranging the single-bit
flags to try to group flags with similar purposes, but eventually
decided that that involved too many judgment calls.
Discussion: https://postgr.es/m/17385-9ee529fb091f0ce5@postgresql.org
Previously, the transaction-property GUCs such as transaction_isolation
could be reset after starting a transaction, because we marked them
as GUC_NO_RESET_ALL but still allowed a targeted RESET. That leads to
assertion failures or worse, because those properties aren't supposed
to change after we've acquired a transaction snapshot.
There are some NO_RESET_ALL variables for which RESET is okay, so
we can't just redefine the semantics of that flag. Instead introduce
a separate GUC_NO_RESET flag. Mark "seed", as well as the transaction
property GUCs, as GUC_NO_RESET.
We have to disallow GUC_ACTION_SAVE as well as straight RESET, because
otherwise a function having a "SET transaction_isolation" clause can
still break things: the end-of-function restore action is equivalent
to a RESET.
No back-patch, as it's conceivable that someone is doing something
this patch will forbid (like resetting one of these GUCs at transaction
start, or "CREATE FUNCTION ... SET transaction_read_only = 1") and not
running into problems with it today. Given how long we've had this
issue and not noticed, the side effects in non-assert builds can't be
too serious.
Per bug #17385 from Andrew Bille.
Masahiko Sawada
Discussion: https://postgr.es/m/17385-9ee529fb091f0ce5@postgresql.org
While at it, remove an unused queryString parameter from
CheckPubRelationColumnList() and make other minor stylistic changes.
Backpatch to 15.
Reported by Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Co-authored-by: Hou zj <houzj.fnst@fujitsu.com>
Discussion: https://postgr.es/m/20220926.160426.454497059203258582.horikyota.ntt@gmail.com
We weren't jumbling the merge action list, so wildly different commands
would be considered to use the same query ID. Add that, mention it in
the docs, and some test lines.
Backpatch to 15.
Author: Tatsu <bt22nakamorit@oss.nttdata.com>
Reviewed-by: Julien Rouhaud <rjuju123@gmail.com>
Discussion: https://postgr.es/m/d87e391694db75a038abc3b2597828e8@oss.nttdata.com
This was used as the returned result type of the generated contents for
the backup_label and backup history files. This is replaced by a simple
string, reducing the cleanup burden of all the callers of
build_backup_content().
Reviewed-by: Bharath Rupireddy
Discussion: https://postgr.es/m/YzERvNPaZivHEKZJ@paquier.xyz
Historically we've been more worried about making the output of
float fields look pretty than whether they'd be read back exactly.
That won't work if we're to compare the read-back nodes for
equality, so switch to using the Ryu code for float output.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
Historically, outToken has represented both NULL and empty-string
strings as "<>", which readfuncs.c then read as NULL, thus failing
to preserve empty-string fields accurately. Remarkably, this has
not caused any serious problems yet, but let's fix it.
We'll keep the "<>" notation for NULL, and use """" for empty string,
because that matches other notational choices already in use.
An actual input string of """" is converted to "\""" (this was true
already, apparently as a hangover from an ancient time when string
quoting was handled directly by pg_strtok).
CHAR fields also use "<>", but for '\0'.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
This change simplifies some of the logic related to the generation and
creation of the backup_label and backup history files, which has become
unnecessarily complicated since the removal of the exclusive backup mode
in commit 39969e2. The code was previously generating the contents of
these files as a string (start phase for the backup_label and stop phase
for the backup history file), one problem being that the contents of the
backup_label string were scanned to grab some of its internal contents
at the stop phase.
This commit changes the logic so as we store the data required to build
these files in an intermediate structure named BackupState. The
backup_label file and backup history file strings are generated when
they are ready to be sent back to the client. Both files are now
generated with the same code path. While on it, this commit renames
some variables for clarity.
Two new files named xlogbackup.{c,h} are introduced in this commit, to
remove from xlog.c some of the logic around base backups. Note that
more could be moved to this new set of files.
Author: Bharath Rupireddy, Michael Paquier
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CALj2ACXWwTDgJqCjdaPyfR7djwm6SrybGcrZyrvojzcsmt4FFw@mail.gmail.com
Commit 25936fd46 adjusted things so that the "storeslot" we use
for remapping trigger tuples would have adequate lifespan, but it
neglected to consider the lifespan of the tuple descriptor that
the slot depends on. It turns out that in at least some cases, the
tupdesc we are passing is a refcounted tupdesc, and the refcount for
the slot's reference can get assigned to a resource owner having
different lifespan than the slot does. That leads to an error like
"tupdesc reference 0x7fdef236a1b8 is not owned by resource owner
SubTransaction". Worse, because of a second oversight in the same
commit, we'd try to free the same tupdesc refcount again while
cleaning up after that error, leading to recursive errors and an
"ERRORDATA_STACK_SIZE exceeded" PANIC.
To fix the initial problem, let's just make a non-refcounted copy
of the tupdesc we're supposed to use. That seems likely to guard
against additional problems, since there's no strong reason for
this code to assume that what it's given is a refcounted tupdesc;
in which case there's an independent hazard of the tupdesc having
shorter lifespan than the slot does. (I didn't bother trying to
free said copy, since it should go away anyway when the (sub)
transaction context is cleaned up.)
The other issue can be fixed by making the code added to
AfterTriggerFreeQuery work like the rest of that function, ie be
sure that it doesn't try to free the same slot twice in the event
of recursive error cleanup.
While here, also clean up minor stylistic issues in the test case
added by 25936fd46: don't use "create or replace function", as any
name collision within the tests is likely to have ill effects
that that won't mask; and don't use function names as generic as
trigger_function1, especially if you're not going to drop them
at the end of the test stanza.
Per bug #17607 from Thomas Mc Kay. Back-patch to v12, as the
previous fix was.
Discussion: https://postgr.es/m/17607-bd8ccc81226f7f80@postgresql.org
Commit 4fb5c794e intended to add coverage of some ambuildempty
methods that were not getting reached, without removing any
test coverage. However, by changing a temp table to unlogged
it managed to negate the intent of 4c51a2d1e, which means that
we didn't have reliable test coverage of ginvacuum.c anymore.
As things stand, much of that file might or might not get reached
depending on timing, which seems pretty undesirable.
Although this is only clearly broken for the GIN test, it seems
best to revert 4fb5c794e altogether and instead add bespoke test
cases covering unlogged indexes for these four AMs. We don't
need to do very much with them, so the extra tests are cheap.
(Note that btree, hash, and bloom already have similar test cases,
so they need no additional work.)
We can also undo dec8ad367. Since the testing deficiency that that
hacked around was later fixed by 2f2e24d90, let's intentionally leave
an unlogged table behind to improve test coverage in the modules that
use the regression database for other test purposes. (The case I used
also leaves an unlogged sequence behind.)
Per report from Alex Kozhemyakin. Back-patch to v15 where the
faulty test came in.
Discussion: https://postgr.es/m/b00c8ee096ee46cd25c183125562a1a7@postgrespro.ru
The node types A_Const, Constraint, and A_Expr had custom output
functions, but no read functions were implemented so far.
The A_Expr output format had to be tweaked a bit to make it easier to
parse.
Be a bit more cautious about applying strncmp to unterminated strings.
Also error out if an unrecognized enum value is found in each case,
instead of just printing a placeholder value. That was maybe ok for
debugging but won't work if we want to have robust round-tripping.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
The node tokenizer went out of its way to store BitString node values
without the leading 'b'. But everything else in the system stores the
leading 'b'. This would break if a BitString node is
read-printed-read.
Also, the node tokenizer didn't know that BitString node tokens could
also start with 'x'.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
The main parser checks whether a literal fits into an int when
deciding whether it should be put into an Integer or Float node. The
parser processes integer literals without signs. So a most-negative
integer literal will not fit into Integer and will end up as a Float
node.
The node tokenizer did this differently. It included the sign when
checking whether the literal fit into int. So a most-negative integer
would indeed fit that way and end up as an Integer node.
In order to preserve the node structure correctly, we need the node
tokenizer to also analyze integer literals without sign.
There are a number of test cases in the regression tests that have a
most-negative integer argument of some utility statement, so this
issue is easily reproduced under WRITE_READ_PARSE_PLAN_TREES.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/4159834.1657405226@sss.pgh.pa.us
There's really no need to build win32ver.rc as part of building
pgmsgevent.rc. This will make it sligthly easier to add rc file generation to
the meson build.
Because index creation does not go through heap_create_with_catalog() we
didn't call pgstat_create_relation(), leading to index stats of a newly
created realtion not getting dropped during rollback. To fix, move the
pgstat_create_relation() to heap_create(), which indexes do use.
Similarly, because dropping an index does not go through
heap_drop_with_catalog(), we didn't drop index stats when the transaction
dropping an index committed. Here there's no convenient common path for
indexes and relations, so index_drop() now calls pgstat_drop_relation().
Add tests for transactional index stats handling.
Author: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/51bbf286-2b4a-8998-bd12-eaae4b765d99@amazon.com
Backpatch: 15-, like 8b1dccd37c, which introduced the bug
The extended query protocol implementation I added in commit
acb7e4eb6b has bugs when used in pipeline mode. Rather than spend
more time trying to fix it, remove that code and make the function rely
on simple query protocol only, meaning it can no longer be used in
pipeline mode.
Users can easily change their applications to use PQsendQueryParams
instead. We leave PQsendQuery in place for Postgres 14, just in case
somebody is using it and has not hit the mentioned bugs; but we should
recommend that it not be used.
Backpatch to 15.
Per bug report from Gabriele Varrazzo.
Discussion: https://postgr.es/m/CA+mi_8ZGSQNmW6-mk_iSR4JZB_LJ4ww3suOF+1vGNs3MrLsv4g@mail.gmail.com
The "emulation" I wrote for PQsendQuery in pipeline mode to use extended
query protocol, in commit acb7e4eb6b, is problematic. Due to numerous
bugs we'll soon remove it. As a first step and for all branches back to
14, stop using PQsendQuery in libpq_pipeline. Also remove a few test
lines that will no longer be relevant.
Backpatch to 14.
Discussion: https://postgr.es/m/CA+mi_8ZGSQNmW6-mk_iSR4JZB_LJ4ww3suOF+1vGNs3MrLsv4g@mail.gmail.com
We previously thought that allowing such cases can confuse users when they
specify DROP TABLES IN SCHEMA but that doesn't seem to be the case based
on discussion. This helps to uplift the restriction during
ALTER TABLE ... SET SCHEMA which used to ensure that we couldn't end up
with a publication having both a schema and the same schema's table.
To allow this, we need to forbid having any schema on a publication if
column lists on a table are specified (and vice versa). This is because
otherwise we still need a restriction during ALTER TABLE ... SET SCHEMA to
forbid cases where it could lead to a publication having both a schema and
the same schema's table with column list.
Based on suggestions by Peter Eisentraut.
Author: Hou Zhijie and Vignesh C
Reviewed-By: Peter Smith, Amit Kapila
Backpatch-through: 15, where it was introduced
Discussion: https://postgr.es/m/2729c9e2-9aac-8cda-f2f4-34f2bcc18f4e@enterprisedb.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in pg_dump/pg_dumpall
related code.
Affected code happens to be inconsistent in how it applies conventions
around how Archive and Archive Handle variables are named. Significant
code churn is required to fully fix those inconsistencies, so take the
least invasive approach possible: treat function definition names as
authoritative, and mechanically adjust corresponding names from function
definitions to match.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAH2-Wzmma+vzcO6gr5NYDZ+sx0G14aU-UrzFutT2FoRaisVCUQ@mail.gmail.com
Make ecpg function declarations consistently use named parameters. Also
make sure that the declarations use names that match corresponding names
from function definitions.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
This may be a bit too subtle, but removing that word from there makes
this clause no longer a perfect parallel of the GRANT variant "ALL
TABLES IN SCHEMA": indeed, for publications what we record is the schema
itself, not the tables therein, which means that any tables added to the
schema in the future are also published. This is completely different
to what GRANT does, which is affect only the tables that exist when the
command is executed.
There isn't resounding support for this change, but there are a few
positive votes and no opposition. Because the time to 15 RC1 is very
short, let's get this out now.
Backpatch to 15.
Discussion: https://postgr.es/m/2729c9e2-9aac-8cda-f2f4-34f2bcc18f4e
The bounds hardcoded in compression.c since ffd5365 (minimum at 1 and
maximum at 22) do not match the reality of what zstd is able to
handle, these values being available via ZSTD_maxCLevel() and
ZSTD_minCLevel() at run-time. The maximum of 22 is actually correct
in recent versions, but the minimum was not as the library can go down
to -131720 by design. This commit changes the code to use the run-time
values in the code instead of some hardcoded ones.
Zstd seems to assume that these bounds could change in the future, and
Postgres will be able to adapt automatically to such changes thanks to
what's being done in this commit.
Reported-by: Justin Prysby
Discussion: https://postgr.es/m/20220922033716.GL31833@telsasoft.com
Backpatch-through: 15
Autoconf is showing its age, fewer and fewer contributors know how to wrangle
it. Recursive make has a lot of hard to resolve dependency issues and slow
incremental rebuilds. Our home-grown MSVC build system is hard to maintain for
developers not using Windows and runs tests serially. While these and other
issues could individually be addressed with incremental improvements, together
they seem best addressed by moving to a more modern build system.
After evaluating different build system choices, we chose to use meson, to a
good degree based on the adoption by other open source projects.
We decided that it's more realistic to commit a relatively early version of
the new build system and mature it in tree.
This commit adds an initial version of a meson based build system. It supports
building postgres on at least AIX, FreeBSD, Linux, macOS, NetBSD, OpenBSD,
Solaris and Windows (however only gcc is supported on aix, solaris). For
Windows/MSVC postgres can now be built with ninja (faster, particularly for
incremental builds) and msbuild (supporting the visual studio GUI, but
building slower).
Several aspects (e.g. Windows rc file generation, PGXS compatibility, LLVM
bitcode generation, documentation adjustments) are done in subsequent commits
requiring further review. Other aspects (e.g. not installing test-only
extensions) are not yet addressed.
When building on Windows with msbuild, builds are slower when using a visual
studio version older than 2019, because those versions do not support
MultiToolTask, required by meson for intra-target parallelism.
The plan is to remove the MSVC specific build system in src/tools/msvc soon
after reaching feature parity. However, we're not planning to remove the
autoconf/make build system in the near future. Likely we're going to keep at
least the parts required for PGXS to keep working around until all supported
versions build with meson.
Some initial help for postgres developers is at
https://wiki.postgresql.org/wiki/Meson
With contributions from Thomas Munro, John Naylor, Stone Tickle and others.
Author: Andres Freund <andres@anarazel.de>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Author: Peter Eisentraut <peter@eisentraut.org>
Reviewed-By: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/20211012083721.hvixq4pnh2pixr3j@alap3.anarazel.de
If the ps display is not cleared at this point, the process could
continue displaying "recovering NNN" even if handling end-of-recovery
steps. df9274a has tackled that by providing some information with the
end-of-recovery checkpoint but 7ff23c6 has nullified the effect of the
first commit.
Per a suggestion from Justin, just clear the ps display when we are done
with recovery, so as no incorrect information is displayed. This may
get extended in the future, but for now restore the pre-7ff23c6
behavior.
Author: Justin Prysby
Discussion: https://postgr.es/m/20220913223954.GU31833@telsasoft.com
Backpatch-through: 15
This commit updates two code paths to use pg_lfind32() introduced by
b6ef167 with TransactionId arrays:
- At the end of TransactionIdIsInProgress(), when checking for the case
of still running but overflowed subxids.
- XidIsConcurrent(), when checking for a serializable conflict.
These cases are less impactful than 37a6e5d, but a bit of
micro-benchmarking of this API shows that linear search speeds up by
~20% depending on the number of items involved (x86_64 and amd64 looked
at here).
Author: Nathan Bossart
Reviewed-by: Richard Guo, Michael Paquier
Discussion: https://postgr.es/m/20220901185153.GA783106@nathanxps13
Commit 7103ebb7aa added the tab-completion for MERGE accidentally
in the middle of that for LOCK TABLE. This commit fixes this issue.
This also adds some tab-completion for MERGE.
Back-patch to v15 where MERGE was introduced.
Author: Kotaro Kawamoto, Fujii Masao
Reviewed-by: Shinya Kato, Álvaro Herrera
Discussion: https://postgr.es/m/9f1ad2a87a58cd5e7d64f3993130958d@oss.nttdata.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions for several "lexer
adjacent" backend functions.
These functions were missed by recent commits because they were obscured
by clang-tidy warnings about functions whose signature is directly under
the control of the lexer (flex seems to always generate function
declarations with unnamed parameters). We probably can't fix most of
the warnings it generates for translation units that get built from .l
and .y files, but we can at least do this much.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
We check that the ICU locale is only specified if the ICU locale
provider is selected. But we did that too early. We need to wait
until we load the settings of the template database, since that could
also set what the locale provider is.
Reported-by: Marina Polyakova <m.polyakova@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru
For publication schemas (OBJECT_PUBLICATION_NAMESPACE) and user
mappings (OBJECT_USER_MAPPING), pg_get_object_address() checked the
array length of the second argument, but not of the first argument.
If the first argument was too long, it would just silently ignore
everything but the first argument. Fix that by checking the length of
the first argument as well.
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/caaef70b-a874-1088-92ef-5ac38269c33b%40enterprisedb.com
Previously the following snprintf() wrappers:
* ReplicationSlotNameForTablesync()
* ReplicationOriginNameForTablesync()
... used int as a second argument of snprintf() while the actual type of it
is size_t. Although it doesn't fail at present better replace it with Size
for consistency with the rest of the system.
Author: Aleksander Alekseev
Reviewed-By: Peter Smith
Discussion: https://postgr.es/m/CAHut%2BPsa8hhfSE6ozUK-ih7GkQziAVAf4f3bqiXEj2nQiu-43g%40mail.gmail.com
Visual Studio 2015+ has support for a macro to control the alignement of
structures as of __declspec(align(#)), and this commit adds a definition
of pg_attribute_aligned() based on that. It happens that this was
already used in the implementation of atomics for MSVC. Note that there
is still no definition fo pg_attribute_packed(), so this does not impact
itemptr.h.
Author: James Coleman
Discussion: https://postgr.es/m/CAAaqYe-HbtZvR3msoMtk+hYW2S0e0OapzMW8icSMYTMA+mN8Aw@mail.gmail.com
expression_tree_walker and allied functions have traditionally
declared their callback functions as, say, "bool (*walker) ()"
to allow for variation in the declared types of the callback
functions' context argument. This is apparently going to be
forbidden by the next version of the C standard, and the latest
version of clang warns about that. In any case it's always
been pretty poor for error-detection purposes, so fixing it is
a good thing to do.
What we want to do is change the callback argument declarations to
be like "bool (*walker) (Node *node, void *context)", which is
correct so far as expression_tree_walker and friends are concerned,
but not change the actual callback functions. Strict compliance with
the C standard would require changing them to declare their arguments
as "void *context" and then cast to the appropriate context struct
type internally. That'd be very invasive and it would also introduce
a bunch of opportunities for future bugs, since we'd no longer have
any check that the correct sort of context object is passed by outside
callers or internal recursion cases. Therefore, we're just going
to ignore the standard's position that "void *" isn't necessarily
compatible with struct pointers. No machine built in the last forty
or so years actually behaves that way, so it's not worth introducing
bug hazards for compatibility with long-dead hardware.
Therefore, to silence these compiler warnings, introduce a layer of
macro wrappers that cast the supplied function name to the official
argument type. Thanks to our use of -Wcast-function-type, this will
still produce a warning if the supplied function is seriously
incompatible with the required signature, without going as far as
the official spec restriction does.
This method fixes the problem without any need for source code changes
outside nodeFuncs.h/.c. However, it is an ABI break because the
physically called functions now have names ending in "_impl". Hence
we can only fix it this way in HEAD. In the back branches, we'll have
to settle for disabling -Wdeprecated-non-prototype.
Discussion: https://postgr.es/m/CA+hUKGKpHPDTv67Y+s6yiC8KH5OXeDg6a-twWo_xznKTcG0kSA@mail.gmail.com
Fix selfuncs.h cpluspluscheck complaint, without reintroducing a
parameter name inconsistency (restore the original declaration names,
and then make corresponding function definitions consistent with that).
Oversight in commit a601366a.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Andres Freund <andres@anarazel.de>
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in optimizer, parser,
utility, libpq, and "commands" code, as well as in remaining library
code. Do the same for all code related to frontend programs (with the
exception of pg_dump/pg_dumpall related code).
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will handle
ecpg and pg_dump/pg_dumpall.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
To avoid duplicating the PG_TEST_EXTRA logic in Makefiles into the upcoming
meson based build definition, move the checks into the the tests
themselves. That also has the advantage of making skipped tests visible.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/7dae5979-c6c0-cec5-7a36-76a85aa8053d@enterprisedb.com
The original `trap` lines in these scripts are incomplete: in case of
any signal, they delete the working directory but let the script run to
completion, which is useless because it will only proceed to complain
about the working directory being removed. Add `exit` there, with the
original exit value (not rm's).
Since this is mostly just cosmetic, no backpatch.
Discussion: https://postgr.es/m/20220913181002.hzsosy7qkemb7ky7@alvherre.pgsql
clang 15+ will issue a set-but-not-used warning when the only
use of a variable is in autoincrements (e.g., "foo++;").
That's perfectly sensible, but it detects a few more cases that
we'd not noticed before. Silence the warnings with our usual
methods, such as PG_USED_FOR_ASSERTS_ONLY, or in one case by
actually removing a useless variable.
One thing that we can't nicely get rid of is that with %pure-parser,
Bison emits "yynerrs" as a local variable that falls foul of this
warning. To silence those, I inserted "(void) yynerrs;" in the
top-level productions of affected grammars.
Per recently-established project policy, this is a candidate
for back-patching into out-of-support branches: it suppresses
annoying compiler warnings but changes no behavior. Hence,
back-patch to 9.5, which is as far as these patches go without
issues. (A preliminary check shows that the prior branches
need some other set-but-not-used cleanups too, so I'll leave
them for another day.)
Discussion: https://postgr.es/m/514615.1663615243@sss.pgh.pa.us
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in storage, catalog,
access method, executor, and logical replication code, as well as in
miscellaneous utility/library code.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will do the
same for other parts of the codebase.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
The motivation for this is twofold. For one the meson patchset would like to
have more control over the logfiles. For another, the log file location for
tap tests (tmp_check/log) is not symmetric to the log location for
pg_regress/isolation tests (log/).
This commit does not change the default location for log files for tap tests,
as that'd break the buildfarm log collection, it just provides the
infrastructure for doing so.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/1131990.1660661896@sss.pgh.pa.us
Discussion: https://postgr.es/m/20220828170806.GN2342@telsasoft.com
This is motivated by the meson patchset, which wants to put the log / data for
tests in a different place than the autoconf build. Right now log files for
tap tests have to be inside $TESTDIR/tmp_check, whereas log files for
pg_regress/isolationtester are outside of tmp_check. This change doesn't fix
the latter, but is a prerequisite.
The only test that needs adjustment is 010_tab_completion.pl, as it hardcoded
the tmp_check/ directory. Instead create a dedicated directory for the test
files. It's also a bit cleaner independently, because it doesn't intermingle
the test files with more important things like the log/ directory.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/1131990.1660661896@sss.pgh.pa.us
Discussion: https://postgr.es/m/d861493c-ed20-c251-7a89-7924f5197341@enterprisedb.com
Make sure that function declarations use names that exactly match the
corresponding names from function definitions. Having parameter names
that are reliably consistent in this way will make it easier to reason
about groups of related C functions from the same translation unit as a
module. It will also make certain refactoring tasks easier.
Like other recent commits that cleaned up function parameter names, this
commit was written with help from clang-tidy. Later commits will do the
same for other parts of the codebase.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
Adjust a handful of remaining function prototypes that were overlooked
by recent commit bc2187ed. This oversight wasn't caught by clang-tidy
because the functions in question are only built in custom REG_DEBUG
builds.
Author: Peter Geoghegan <pg@bowt.ie>
Reported-By: Tom Lane <tgl@sss.pgh.pa.us>
The main issue with using gendef.pl as-is for meson is that with meson the
filenames are a bit longer, exceeding the max commandline length when calling
dumpbin with all objects. As it's easier to pass in a library anyway, do so.
The .def file location, input and temporary file location need to be tunable
as well.
This also fixes a bug in gendef.pl: The logic when to regenerate was broken
and never avoid regenerating.
Author: Andres Freund <andres@anarazel.de>
Reviewed-By: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/20220809071055.rgikv3qn74ypnnbb@awork3.anarazel.de
Discussion: https://postgr.es/m/7dae5979-c6c0-cec5-7a36-76a85aa8053d@enterprisedb.com
Make our copy of the IANA timezone library use named parameters in
function declarations. Also make sure that parameter names from each
function's declaration match corresponding definition parameter names.
This makes the timezone code follow Postgres coding standards. The
value of having a consistent standard everywhere seems to outweigh the
cost of keeping the function declarations in sync with future IANA
releases.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
Make regex code consistently use named parameters in function
declarations. Also make sure that parameter names from each function's
declaration match corresponding definition parameter names.
This makes Henry Spencer's regex code follow Postgres coding standards.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
The header comment for get_cheapest_group_keys_order() claimed that the
output arguments were set to a newly allocated list which may be freed by
the calling function, however, this was not always true as the function
would simply leave these arguments untouched in some cases.
This tripped me up when working on 1349d2790 as I mistakenly assumed I
could perform a list_concat with the output parameters. That turned out
bad due to list_concat modifying the original input lists.
In passing, make it more clear that the number of distinct values is
important to reduce tiebreaks during sorts. Also, explain what the
n_preordered parameter means.
Backpatch-through: 15, where get_cheapest_group_keys_order was introduced.
If role A is a direct or indirect member of role B but does not inherit
B's privileges (because at least one relevant grant was created WITH
INHERIT FALSE) then A should not be permitted to bypass privilege
checks that require the privileges of B. For example, A can't change
the privileges of objects owned by B, nor can A drop those objects.
However, up until now, it's been possible for A to change default
privileges for role B. That doesn't seem to be correct, because a
non-inherited role grant is only supposed to permit you to assume
the identity of the granted role via SET ROLE, and should not
otherwise permit you to exercise the privileges of that role. Most
places followed that rule, but this case was an exception.
This could be construed as a security vulnerability, but it does not
seem entirely clear cut, since older branches were fuzzy about the
distinction between is_member_of_role() and has_privs_of_role() in
a number of other ways as well. Because of this, and because
user-visible behavior changes in minor releases are to be avoided
whenever possible, no back-patch.
Discussion: http://postgr.es/m/CA+TgmobG_YUP06R_PM_2Z7wR0qv_52gQPHD8CYXbJva0cf0E+A@mail.gmail.com
Normally when we use object-oriented programming techniques, we
provide a pointer to an object and then some way of looking up the
associated table of callbacks, but walmethods.c/h took the alternative
approach of providing only a pointer to the table of callbacks and
thus imposed the artificial restriction that there could only ever be
one object of each type, so that the callbacks could find it via a
global variable. That doesn't seem like the right idea, so revise the
approach.
Each callback which does not already have an argument of type
Walfile * now takes a pointer to the relevant WalWriteMethod *
so that these callbacks need not rely on there being only one
object of each type.
Freeing a WalWriteMethod is now performed via a callback provided
for that purpose rather than requiring the caller to know which
WAL method they want to free.
Discussion: http://postgr.es/m/CA+TgmoZS0Kw98fOoAcGz8B9iDhdqB4Be4e=vDZaJZ5A-xMYBqA@mail.gmail.com
The API contract for planstate_tree_walker() callbacks is that they
take a PlanState pointer and a context pointer. Somebody figured
they could save a couple lines of code by ignoring that, and passing
ExecShutdownNode itself as the walker even though it has but one
argument. Somewhat remarkably, we've gotten away with that so far.
However, it seems clear that the upcoming C2x standard means to
forbid such cases, and compilers that actively break such code
likely won't be far behind. So spend the extra few lines of code
to do it honestly with a separate walker function.
In HEAD, we might as well go further and remove ExecShutdownNode's
useless return value. I left that as-is in back branches though,
to forestall complaints about ABI breakage.
Back-patch, with the thought that this might become of practical
importance before our stable branches are all out of service.
It doesn't seem to be fixing any live bug on any currently known
platform, however.
Discussion: https://postgr.es/m/208054.1663534665@sss.pgh.pa.us
This makes the curent file position and pathname visible in a generic
way, so we no longer need current_walfile_name global variable or the
get_current_pos() method. Since that purported to be able to fail but
never actually did, this also lets us get rid of some unnecessary
error-handling code.
One risk of this change is that the get_current_pos() method
previously cleared the error indicator, and that will no longer happen
with the new approach. I looked for a way that this could cause problems
and did not find one.
The previous code was confused about whether "Walfile" was the
implementation-dependent structure representing a WAL file or
whether it was a pointer to that stucture. Some of the code used it
one way, and some in the other. The compiler tolerated that because
void * is interchangeable with void **, but now that Walfile is a
struct, it's necessary to be consistent. Hence, some references to
"Walfile" have been converted to "Walfile *".
Discussion: http://postgr.es/m/CA+TgmoZS0Kw98fOoAcGz8B9iDhdqB4Be4e=vDZaJZ5A-xMYBqA@mail.gmail.com
Since c7aba7c, the transform method used during parse analysis of a
subcripting construct has moved from transformAssignmentSubscripts() to
the per-type transform method (arrays or arbitrary types) the step that
checks for slicing support, but transformAssignmentSubscripts() has kept
traces of it. Removing it simplifies the code, so let's clean up all
that.
Author: Zhang Mingli
Reviewed-by: Richard Guo
Discussion: https://postgr.es/m/0d7041ac-c704-48ad-86fd-e05feddf08ed@Spark
Make reorderbuffer.h function declarations consistently use named
parameters. Also make sure that the declarations use names that match
corresponding names from function definitions in reorderbuffer.c. This
makes the definitions easier to follow, especially in the case of
functions that happen to have adjoining arguments of the same type.
This patch was written with help from clang-tidy. Specifically, its
"readability-inconsistent-declaration-parameter-name" check and its
"readability-named-parameter" check were used.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/3955318.1663377656@sss.pgh.pa.us
The function has a bool argument named "case_insensitive", but that was
spelled "case_sensitive" in the declaration. Make them consistent now
to avoid confusion in the future.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Michael Paquiër <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
Backpatch: 10-
So far they were created below CacheMemoryContext. However, that's not
guaranteed to exist in all situations, leading to memory contexts created as
top-level contexts. There isn't actually a good reason anymore to create them
below CacheMemoryContext, so just creating them below TopMemoryContext seems
the best approach.
Reported-by: Reid Thompson <reid.thompson@crunchydata.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Author: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Discussion: https://postgr.es/m/b948b729-42fe-f88c-2f4a-0e65d84c049b@amazon.com
Backpatch: 15-
Since Windows 10 1703, it is additionally necessary to pass a flag
called FILE_MAP_LARGE_PAGES to MapViewOfFile() to enable large pages at
map time. This flag is ignored on older versions of Windows, where
large pages should still be able to work properly without setting it.
Note that the flag would be set only for binaries that knew about it at
compile-time, which should be more or less all the Windows environments
these days.
Since 495ed0e, Windows 10 is the minimum version of Windows supported by
Postgres, making this change easy to reason about on HEAD. Per
discussion, no backpatch is done for the moment.
Reported-by: Okano Naoki
Author: Thomas Munro
Reviewed-by: Tom Lane, Michael Paquier, Julien Rouhaud
Discussion: https://postgr.es/m/17448-0a96583a67edb1f7@postgresql.org
Very occasionally the stats test failed due to the number of sessions not
being updated yet. Likely this requires that there is contention on the
database's stats entry. Solve this by forcing pending stats to be flushed
before fetching the stats.
I verified that there are no other test failures after making
pgstat_report_stat() only flush stats when force = true.
Per message from Tom Lane and buildfarm member crake.
Discussion: https://postgr.es/m/3428246.1663271992@sss.pgh.pa.us
Backpatch: 15-, where 5264add784 added the test
Treat arguments declared as RECORD as if that were a polymorphic type
(which it is, sort of), in that we substitute the actual argument type
while forming the function cache lookup key. This allows the specific
composite type to be known in some cases where it was not before,
at the cost of making a separate function cache entry for each named
composite type that's passed to the function during a session. The
particular symptom discussed in bug #17610 could be solved in other
more-efficient ways, but only at the cost of considerable development
work, and there are other cases where we'd still fail without this.
Per bug #17610 from Martin Jurča. Back-patch to v11 where we first
allowed plpgsql functions to be declared as taking type RECORD.
Discussion: https://postgr.es/m/17610-fb1eef75bf6c2364@postgresql.org
For some reason we'd never decorated pg_v*printf() with
pg_attribute_printf() annotations. There is a convention for
how to label va_list-using printf functions (write zero for the
second argument), and we use that liberally elsewhere in the
code, but these core functions lacked it. It's not clear how
much useful checking the compiler can do for calls of these,
but we might as well add the annotations.
Also, sync win32security.c's log_error() with our normal convention
that pg_attribute_printf must be attached to a function's declaration
not definition. Apparently this file is only compiled with compilers
that aren't picky about that, but still it'd be better to be
consistent.
No back-patch since there's little reason to think we would catch
anything.
Discussion: https://postgr.es/m/3492412.1663283395@sss.pgh.pa.us
If the createdb tests run under the C locale, the database cluster
will be initialized with encoding SQL_ASCII. With the checks added in
c7db01e325, this will cause several
ICU-related tests to fail because SQL_ASCII is not supported by ICU.
To work around that, use initdb option -E UTF8 for those tests to get
past that check.
Check in CREATE DATABASE and initdb that the selected encoding is
supported by ICU. Before, they would pass but users would later get
an error from the server when they tried to use the database.
Also document that initdb sets the encoding to UTF8 by default if the
ICU locale provider is chosen.
Author: Marina Polyakova <m.polyakova@postgrespro.ru>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/6dd6db0984d86a51b7255ba79f111971@postgrespro.ru
I happened to notice that libpq_pipeline's private implementation
of pg_fatal lacked any pg_attribute_printf decoration. Indeed,
adding that turned up a mistake! We'd likely never have noticed
because the error exits in this code are unlikely to get hit,
but still, it's a bug.
We're so used to having the compiler check this stuff for us that
a printf-like function without pg_attribute_printf is a land mine.
I wonder if there is a way to detect such omissions.
Back-patch to v14 where this code came in.
Commit 31dcfae83 changed one pg_resetwal output string, and a
corresponding test in pg_upgrade, without sufficient thought for
the consequences. We can't change that output without creating
hazards for cross-version upgrades, since pg_upgrade needs to be able
to read the output of several different versions of pg_resetwal.
There may well be external tools with the same requirement.
For the moment, just revert those two changes. What we really
ought to do here is have a separate, stable, easily machine-readable
output format for pg_resetwal and pg_controldata, as proposed
years ago by Alvaro. Once that's in place and tools no longer
need to depend on the exact spelling of the human-readable output,
we can put back this change.
Discussion: https://postgr.es/m/fbea8c6f-415a-bad9-c3de-969c40d08a84@dunslane.net
After commit cc2c7d65fc added this flag,
failure to reset it caused assertion failures. In non-assert builds, it
made the system fail to achieve the objectives listed in that commit;
chiefly, we might emit a spurious log message. Back-patch to v15, where
that commit first appeared.
Bharath Rupireddy and Kyotaro Horiguchi. Reviewed by Dilip Kumar,
Nathan Bossart and Michael Paquier. Reported by Dilip Kumar.
Discussion: https://postgr.es/m/CAFiTN-sE3ry=ycMPVtC+Djw4Fd7gbUGVv_qqw6qfzp=JLvqT3g@mail.gmail.com
Commit ecaf7c5df5 removed gram.h from the backend's generated-headers
target. In LLVM builds, this leads to loss of dependency information
when generating .bc files. To fix, add a rule that mirrors ad-hoc .o
dependencies for .bc files as well.
Per cfbot (no buildfarm failures reported)
Analysis by Tom Lane and Andres Freund
Proposed fix by Andres Freund
Discussion: https://www.postgresql.org/message-id/20220914210427.y26tkagmxo5wwbvp%40awork3.anarazel.de
Referring to the WAL as just "log" invites confusion with the
postmaster log, so avoid doing that in docs and error messages.
Also shorten "WAL segment file" to just "WAL file" in various
places.
Bharath Rupireddy, reviewed by Nathan Bossart and Kyotaro Horiguchi
Discussion: https://postgr.es/m/CALj2ACUeXa8tDPaiTLexBDMZ7hgvaN+RTb957-cn5qwv9zf-MQ@mail.gmail.com
In 29f45e299, we added support for optimizing the execution of NOT
IN(values) by using a hash table instead of a linear search over the
array. That commit neglected to update the header comment for
convert_saop_to_hashed_saop() to mention this fact. Here we fix that.
Author: James Coleman
Discussion: https://postgr.es/m/CAAaqYe99NUpAPcxgchGstgM23fmiGjqQPot8627YgkBgNt=BfA@mail.gmail.com
Backpatch-through: 15, where 29f45e299 was added.
Various bits of code were declaring signal handlers manually,
using "int signum" or variants of that. We evidently have no
platforms where that's actually wrong, but let's use our
SIGNAL_ARGS macro everywhere anyway. If nothing else, it's
good for finding signal handlers easily.
No need for back-patch, since this is just cosmetic AFAICS.
Discussion: https://postgr.es/m/2684964.1663167995@sss.pgh.pa.us
In pg_receivewal, compressed output is only flushed on clean exits. The
reason to support SIGTERM as well as SIGINT (which is currently handled)
is that pg_receivewal might well be running as a daemon, and systemd's
default KillSignal is SIGTERM.
Since pg_recvlogical is also supposed to run as a daemon, teach it about
SIGTERM as well and update the documentation to match. While in there,
change pg_receivewal's time_to_stop to be sig_atomic_t like it is in
pg_recvlogical.
Author: Christoph Berg <myon@debian.org>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/Yvo/5No5S0c4EFMj@msg.df7cb.de
This appears to be a merge mistake in 96ef3237bf. We could put it
back the way it was before JSON_TABLE and it'd be two lines shorter, but
it's likely that JSON_TABLE will be back and will prefer things this
way. It makes no other difference in practice.
Backpatch to 15.
Reported by Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAr4nOcNQskC4oBEZN4S+4heJ=1ch_ZKOxU+_Ef-FQSf-g@mail.gmail.com
The zlib documentation mentions the values supported for the compression
strategy, but this code has been using a hardcoded value of 0 rather
than Z_DEFAULT_STRATEGY. This commit adjusts the code to use
Z_DEFAULT_STRATEGY.
Backpatch down to where this code has been added to ease the backport of
any future patch touching this area.
Reported-by: Tom Lane
Discussion: https://postgr.es/m/1400032.1662217889@sss.pgh.pa.us
Backpatch-through: 10
The oldest vendor-shipped Perl in the buildfarm is 5.14.2, which is
the last version that Debian Wheezy shipped. That OS is EOL, but we
keep it running because there is no other convenient way to test certain
non-mainstream 32-bit platforms. There is no bugfix in the 5.14.2 release
that is required, and yet it's also not the latest minor release --
that would be 5.14.4. To clarify the situation, we have thus arranged the
buildfarm to test 5.14.0. That allows configure scripts and documentation
to state 5.14 without fine print.
The MSVC build didn't check the version, since our previous minimum 5.8.3
was considered too old to check for on Windows. We will need a check for
Windows sometime during the v16 cycle, but that could be rendered moot
by the impending Meson conversion, so it seems safe to just document
the requirement for now.
Reviewed by Tom Lane
Discussion: https://www.postgresql.org/message-id/20220902181553.ev4pgzhubhdkguuv@awork3.anarazel.de
This header is semi-private, being used only in files related to
raw parsing, so move to the backend directory where those files
live. This allows removal of Makefile rules that symlink gram.h to
src/include/parser, since gramparse.h can now include gram.h from
within the same directory. This has the side-effect of no longer
installing gram.h and gramparse.h, but there doesn't seem to be a
good reason to continue doing so.
Per suggestion from Andres Freund and Peter Eisentraut
Discussion: https://www.postgresql.org/message-id/20220904181759.px6uosll6zbxcum5%40awork3.anarazel.de
PG_COMPRESSION_OPTION_LEVEL is removed from the compression
specification logic, and instead the compression level is always
assigned with each library's default if nothing is directly given. This
centralizes the checks on the compression methods supported by a given
build, and always assigns a default compression level when parsing a
compression specification. This results in complaining at an earlier
stage than previously if a build supports a compression method or not,
aka when parsing a specification in the backend or the frontend, and not
when processing it. zstd, lz4 and zlib are able to handle in their
respective routines setting up the compression level the case of a
default value, hence the backend or frontend code (pg_receivewal or
pg_basebackup) has now no need to know what the default compression
level should be if nothing is specified: the logic is now done so as the
specification parsing assigns it. It can also be enforced by passing
down a "level" set to the default value, that the backend will accept
(the replication protocol is for example able to handle a command like
BASE_BACKUP (COMPRESSION_DETAIL 'gzip:level=-1')).
This code simplification fixes an issue with pg_basebackup --gzip
introduced by ffd5365, where the tarball of the streamed WAL segments
would be created as of pg_wal.tar.gz with uncompressed contents, while
the intention is to compress the segments with gzip at a default level.
The origin of the confusion comes from the handling of the default
compression level of gzip (-1 or Z_DEFAULT_COMPRESSION) and the value of
0 was getting assigned, which is what walmethods.c would consider
as equivalent to no compression when streaming WAL segments with its tar
methods. Assigning always the compression level removes the confusion
of some code paths considering a value of 0 set in a specification as
either no compression or a default compression level.
Note that 010_pg_basebackup.pl has to be adjusted to skip a few tests
where the shape of the compression detail string for client and
server-side compression was checked using gzip. This is a result of the
code simplification, as gzip specifications cannot be used if a build
does not support it.
Reported-by: Tom Lane
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/1400032.1662217889@sss.pgh.pa.us
Backpatch-through: 15
guc.c has grown to be one of our largest .c files, making it
a bottleneck for compilation. It's also acquired a bunch of
knowledge that'd be better kept elsewhere, because of our not
very good habit of putting variable-specific check hooks here.
Hence, split it up along these lines:
* guc.c itself retains just the core GUC housekeeping mechanisms.
* New file guc_funcs.c contains the SET/SHOW interfaces and some
SQL-accessible functions for GUC manipulation.
* New file guc_tables.c contains the data arrays that define the
built-in GUC variables, along with some already-exported constant
tables.
* GUC check/assign/show hook functions are moved to the variable's
home module, whenever that's clearly identifiable. A few hard-
to-classify hooks ended up in commands/variable.c, which was
already a home for miscellaneous GUC hook functions.
To avoid cluttering a lot more header files with #include "guc.h",
I also invented a new header file utils/guc_hooks.h and put all
the GUC hook functions' declarations there, regardless of their
originating module. That allowed removal of #include "guc.h"
from some existing headers. The fallout from that (hopefully
all caught here) demonstrates clearly why such inclusions are
best minimized: there are a lot of files that, for example,
were getting array.h at two or more levels of remove, despite
not having any connection at all to GUCs in themselves.
There is some very minor code beautification here, such as
renaming a couple of inconsistently-named hook functions
and improving some comments. But mostly this just moves
code from point A to point B and deals with the ensuing
needs for #include adjustments and exporting a few functions
that previously weren't exported.
Patch by me, per a suggestion from Andres Freund; thanks also
to Michael Paquier for the idea to invent guc_funcs.c.
Discussion: https://postgr.es/m/587607.1662836699@sss.pgh.pa.us
Commit 3a0e385048 introduced a new path for unauthenticated bytes from
the client certificate to be printed unescaped to the logs. There are a
handful of these already, but it doesn't make sense to keep making the
problem worse. \x-escape any unprintable bytes.
The test case introduces a revoked UTF-8 certificate. This requires the
addition of the `-utf8` flag to `openssl req`. Since the existing
certificates all use an ASCII subset, this won't modify the existing
certificates' subjects if/when they get regenerated; this was verified
experimentally with
$ make sslfiles-clean
$ make sslfiles
Unfortunately the test can't be run in the CI yet due to a test timing
issue; see 55828a6b60.
Author: Jacob Champion <jchampion@timescale.com>
Discussion: https://www.postgresql.org/message-id/CAAWbhmgsvHrH9wLU2kYc3pOi1KSenHSLAHBbCVmmddW6-mc_=w@mail.gmail.com
Locale options can be specified for initdb, createdb, and CREATE
DATABASE. In initdb, it has always been possible to specify --locale
and then some --lc-* option to override a category. CREATE DATABASE
and createdb didn't allow that, requiring either the all-categories
option or only per-category options. In
f2553d4306, this was changed in CREATE
DATABASE (perhaps by accident?) to be more like the initdb behavior,
but createdb still had the old behavior.
Now we change createdb to match the behavior of CREATE DATABASE and
initdb, and also update the documentation of CREATE DATABASE to match
the new behavior, which was not done in the above commit.
Author: Marina Polyakova <m.polyakova@postgrespro.ru>
Reviewed-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://www.postgresql.org/message-id/7c99c132dc9c0ac630e0127f032ac480@postgrespro.ru
The two strings are already a single palloc'd chunk, not freed; there's
no reason to allocate separate copies that have the same lifetime.
This code is only called in short-lived memory contexts (except in some
cases in TopTransactionContext, which is still short-lived enough not to
really matter), and typically only for short arrays, so the memory or
computation saved is likely negligible. However, let's fix it to avoid
leaving a bad example of code to copy. This is the only place I could
find where we're doing this with makeDefElem().
Reported-by: Junwang Zhao <zhjwpku@gmail.com>
Discussion: https://postgr.es/m/20220909142050.3vv2hjekppk265dd@alvherre.pgsql
Commit d8594d123 updated the list of non-spacing codepoints used
for calculating display width, but in doing so inadvertently removed
some, since the script used for that commit only considered combining
characters.
For complete coverage for zero-width characters, include codepoints in
the category Cf (Format). To reflect the wider purpose, also rename files
and update comments that referred specifically to combining characters.
Some of these ranges have been missing since v12, but due to lack of
field complaints it was determined not important enough to justify adding
special-case logic the backbranches.
Kyotaro Horiguchi
Report by Pavel Stehule
Discussion: https://www.postgresql.org/message-id/flat/CAFj8pRBE8yvpQ0FSkPCoe0Ny1jAAsAQ6j3qMgVwWvkqAoaaNmQ%40mail.gmail.com
This change concerns a couple of .txt files (for internal state checks)
that were still written in the path where the binary is executed, and
not in the subdirectory located in the target cluster. Like the other
.txt files doing already so (like loadable_libraries.txt), these are
saved in the base output directory. Note that on failure, the logs
report the full path to the .txt file generated, so these are easy to
find.
Oversight in 38bfae3.
Author: Daniel Gustafsson
Reviewed-by: Michael Paquier, Justin Prysby
Discussion: https://postgr.es/m/181A6DA8-3B7F-4B71-82D5-363FF0146820@yesql.se
Backpatch-through: 15
The primary fix here is to fix has_matching_range() so it does not
reference ranges->values[-1] when nranges == 0. Similar problems existed
in AssertCheckRanges() too. It does not look like any of these problems
could lead to a crash as the array in question is at the end of the Ranges
struct, and values[-1] is memory that belongs to other fields in the
struct. However, let's get rid of these rather unsafe coding practices.
In passing, I (David) adjusted some comments to try to make it more clear
what some of the fields are for in the Ranges struct. I had to study the
code to find out what nsorted was for as I couldn't tell from the
comments.
Author: Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAqJQzPitufX-jR=YUbJafpCDAKUnwgdbX_MzSc93wuvdw@mail.gmail.com
Backpatch-through: 14, where multi-range brin was added.
Commit c4c340088 changed geometric operators to use float4 and float8
functions, and handle NaN's in a better way. The circle sameness test
had a typo in the code which resulted in all comparisons with the left
circle having a NaN radius considered same.
postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
?column?
----------
t
(1 row)
This fixes the sameness test to consider the radius of both the left
and right circle.
Backpatch to v12 where this was introduced.
Author: Ranier Vilela <ranier.vf@gmail.com>
Discussion: https://postgr.es/m/CAEudQAo8dK=yctg2ZzjJuzV4zgOPBxRU5+Kb+yatFiddtQk6Rw@mail.gmail.com
Backpatch-through: v12
In commit f6c5edb8ab, we started to drop the replication origin slots
before tablesync worker exits to avoid consuming more slots than required.
We were dropping the replication origin in the same transaction where we
were marking the tablesync state as SYNCDONE. Now, if there is any error
after we have dropped the origin but before we commit the containing
transaction, the in-memory state of replication progress won't be rolled
back. Due to this, after the restart, tablesync worker can start streaming
from the wrong location and can apply the already processed transaction.
To fix this, we need to opportunistically drop the origin after marking
the tablesync state as SYNCDONE. Even, if the tablesync worker fails to
remove the replication origin before exit, the apply worker ensures to
clean it up afterward.
Reported by Tom Lane as per buildfarm.
Diagnosed-by: Masahiko Sawada
Author: Hou Zhijie
Reviewed-By: Masahiko Sawada, Amit Kapila
Discussion: https://postgr.es/m/20220714115155.GA5439@depesz.com
Discussion: https://postgr.es/m/CAD21AoAw0Oofi4kiDpJBOwpYyBBBkJj=sLUOn4Gd2GjUAKG-fw@mail.gmail.com
This adds additional variants of palloc, pg_malloc, etc. that
encapsulate common usage patterns and provide more type safety.
Specifically, this adds palloc_object(), palloc_array(), and
repalloc_array(), which take the type name of the object to be
allocated as its first argument and cast the return as a pointer to
that type. There are also palloc0_object() and palloc0_array()
variants for initializing with zero, and pg_malloc_*() variants of all
of the above.
Inspired by the talloc library.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/bb755632-2a43-d523-36f8-a1e7a389a907@enterprisedb.com
This change impacts the backend-side code in charge of starting a LDAP
TLS session. It is a bit sad that it is not possible to unify the WIN32
and non-WIN32 code paths, but the different number of arguments for both
discard this possibility.
This is similar to 47bd0b3, where this replaces the last function
loading that seems worth it, any others being either environment or
version-dependent.
Reported-by: Thomas Munro
Reviewed-by: Thomas Munro
Discussion: https://postgr.es/m/Yx0rxpNgDh8tN4XA@paquier.xyz
The LDAP wiki states that the search message should be freed regardless
of the return value of ldap_search_s(), but we failed to do so in one
backend code path when searching LDAP with a filter. This is not
critical in an authentication code path failing in the backend as this
causes such the process to exit promptly, but let's be clean and free
the search message appropriately, as documented by upstream.
All the other code paths failing a LDAP operation do that already, and
somebody looking at this code in the future may miss what LDAP expects
with the search message.
Author: Zhihong Yu
Discussion: https://postgr.es/m/CALNJ-vTf5Y+8RtzZ4GjOGE9qWVHZ8awfhnFYc_qGm8fMLUNRAg@mail.gmail.com
When building a shared library with exports.txt there's no need to build an
intermediary static library, we can just pass -Wl,-bE:... when generating the
.so.
When building a shared library without exports.txt, there's no need to call
mkldexport.sh to export all symbols, because all symbols are exported anyway,
and we don't need the export file on the import side (like we do for
postgres.imp).
This makes building .so's on aix a lot more similar to building on other
platforms. In particular, we don't create and remove a .a of the same name but
different contents anymore.
Discussion: https://postgr.es/m/20220820174213.d574qde4ptwdzoqz@awork3.anarazel.de
The ECPG preprocessor converted code such as
static varchar str1[10], str2[20], str3[30];
into
static struct varchar_1 { int len; char arr[ 10 ]; } str1 ;
struct varchar_2 { int len; char arr[ 20 ]; } str2 ;
struct varchar_3 { int len; char arr[ 30 ]; } str3 ;
thus losing the storage attribute for the later variables.
Repeat the declaration for each such variable.
(Note that this occurred only for variables declared "varchar"
or "bytea", which may help explain how it escaped detection
for so long.)
Andrey Sokolov
Discussion: https://postgr.es/m/942241662288242@mail.yandex.ru
Because of inadequate filtering, the check triggers were confusing the
search for action triggers in GetForeignKeyActionTriggers and vice-versa
in GetForeignKeyCheckTriggers; this confusion results in seemingly
random assertion failures, and can have real impact in non-asserting
builds depending on catalog order. Change these functions so that they
correctly ignore triggers that are not relevant to each side.
To reduce the odds of further problems, do not break out of the
searching loop in assertion builds. This break is likely to hide bugs;
without it, we would have detected this bug immediately.
This problem was introduced by f4566345cf, so backpatch to 15 where
that commit first appeared.
Author: Amit Langote <amitlangote09@gmail.com>
Discussion: https://postgr.es/m/20220908172029.sejft2ppckbo6oh5@awork3.anarazel.de
Discussion: https://postgr.es/m/4104619.1662663056@sss.pgh.pa.us
Since the retirement of some older buildfarm members, the oldest Bison
that gets regular testing is 2.3. MacOS ships that version, and will
continue doing so for the forseeable future because of Apple's policy
regarding GPLv3. While Mac users could use a package manager to install
a newer version, there is no compelling reason to force them do so at
this time.
Reviewed by Andres Freund
Discussion: https://www.postgresql.org/message-id/1097762.1662145681@sss.pgh.pa.us
This commit changes the following code paths to do direct system calls
to some WIN32 functions rather than loading them from an external
library, shaving some code in the process:
- Creation of restricted tokens in pg_ctl.c, introduced by a25cd81.
- QuerySecurityContextToken() in auth.c for SSPI authentication in the
backend, introduced in d602592.
- CreateRestrictedToken() in src/common/. This change is similar to the
case of pg_ctl.c.
Most of these functions were loaded rather than directly called because,
as mentioned in the code comments, MinGW headers were not declaring
them. I have double-checked the recent MinGW code, and all the
functions changed here are declared in its headers, so this change
should be safe. Note that I do not have a MinGW environment at hand so
I have not tested it directly, but that MSVC was fine with the change.
The buildfarm will tell soon enough if this change is appropriate or not
for a much broader set of environments.
A few code paths still use GetProcAddress() to load some functions:
- LDAP authentication for ldap_start_tls_sA(), where I am not confident
that this change would work.
- win32env.c and win32ntdll.c where we have a per-MSVC version
dependency for the name of the library loaded.
- crashdump.c for MiniDumpWriteDump() and EnumDirTree(), where direct
calls were not able to work after testing.
Reported-by: Thomas Munro
Reviewed-by: Justin Prysby
Discussion: https://postgr.es/m/CA+hUKG+BMdcaCe=P-EjMoLTCr3zrrzqbcVE=8h5LyNsSVHKXZA@mail.gmail.com
On failure in restoring a block image, no details were provided, while
it is possible to see failure with an inconsistent record state, a
failure in processing decompression or a failure in decompression
because a build does not support this option.
RestoreBlockImage() is used in two code paths in the backend code,
during recovery and when checking a page consistency after applying
masking, and both places are changed to consume the error message
produced by the internal routine when it returns a false status. All
the error messages are reported under ERRCODE_INTERNAL_ERROR, that gets
used also when attempting to access a page compressed by a method
not supported by the build attempting the decompression. This is
something that can happen in core when doing physical replication with
primary and standby using inconsistent build options, for example.
This routine is available since 2c03216d and it has never provided any
context about the error happening when it failed. This change is
justified even more after 57aa5b2, that introduced compression of FPWs
in WAL.
Reported-by: Justin Prysby
Author: Michael Paquier
Discussion: https://postgr.es/m/20220905002320.GD31833@telsasoft.com
Backpatch-through: 15
Add a new line to log reports from autovacuum (as well as VACUUM VERBOSE
output) that shows information about freezing. Emphasis is placed on
the total number of heap pages that had one or more tuples frozen by
VACUUM. The total number of tuples frozen is also shown.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Jeff Janes <jeff.janes@gmail.com>
Discussion: https://postgr.es/m/CAH2-WznTY6D0zyE8VLrC6Gd4kh_HGAXxnTPtcOQOOsxzLx9zog@mail.gmail.com
5265e91fd changed MemoryContextContains to update it so that it works
correctly with the new MemoryChunk code added in c6e0fe1f2. However,
5265e91fd was done with the assumption that MemoryContextContains would
only ever be given pointers to memory that had been returned by one of our
MemoryContext allocators. It seems that's not true and many of our 32-bit
buildfarm animals are clearly showing that.
There are some code paths that call MemoryContextContains with a pointer
pointing part way into an allocated chunk. The example of this found by
the 32-bit buildfarm animals is the int2int4_sum() function. This
function returns transdata->sum, which is not a pointer to memory that was
allocated directly. This return value is then subsequently passed to
MemoryContextContains which causes it to crash due to it thinking the
memory directly prior to that pointer is a MemoryChunk. What's actually
in that memory is the field in the struct that comes prior to the "sum"
field. This problem didn't occur in 64-bit world because BIGINT is a
byval type and the code which was calling MemoryContextContains with the
bad pointer only does so with non-byval types.
Here, instead of reverting 5265e91fd and making MemoryContextContains
completely broken again, let's just make it always return false for now.
Effectively prior to 5265e91fd it was doing that anyway, this at least
makes that more explicit. The only repercussions of this with the current
MemoryContextContains calls are that we perform a datumCopy() when we
might not need to. This should make the 32-bit buildfarm animals happy
again and give us more time to consider a long-term fix.
Discussion: https://postgr.es/m/20220907130552.sfjri7jublfxyyi4%40jrouhaud
During ALTER TABLE ATTACH PARTITION, if the name of a parent's foreign
key constraint is already used on the partition, the code tries to
choose another one before the FK attributes list has been populated,
so the resulting constraint name was "<relname>__fkey" instead of
"<relname>_<attrs>_fkey". Repair, and add a test case.
Backpatch to 12. In 11, the code to attach a partition was not smart
enough to cope with conflicting constraint names, so the problem doesn't
exist there.
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Discussion: https://postgr.es/m/20220901184156.738ebee5@karst
We should process completed IOs *before* trying to start more, so that
it is always possible to decode one more record when the decoded record
queue is empty, even if maintenance_io_concurrency is set so low that a
single earlier WAL record might have saturated the IO queue.
That bug was hidden because the effect of maintenance_io_concurrency was
arbitrarily clamped to be at least 2. Fix the ordering, and also remove
that clamp. We need a special case for 0, which is now treated the same
as recovery_prefetch=off, but otherwise the number is used directly.
This allows for testing with 1, which would have made the problem
obvious in simple test scenarios.
Also add an explicit error message for missing contrecords. It was a
bit strange that we didn't report an error already, and became a latent
bug with prefetching, since the internal state that tracks aborted
contrecords would not survive retrying, as revealed by
026_overwrite_contrecord.pl with this adjustment. Reporting an error
prevents that.
Back-patch to 15.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/20220831140128.GS31833%40telsasoft.com
perltidying a "##no critic" line moves the marker to where it becomes
useless. Put the line back to how it was, and protect it from further
malfeasance.
Per buildfarm member crake.
Previously Catalog.pm eval'd each individual hash reference
so that comments and whitespace can be preserved when running
reformat-dat-files. This is unnecessary when building, and we can save
~15% off the run time of genbki.pl by simply slurping and eval'-ing
the whole file at once. This saves a bit of time, especially in highly
parallel builds, since most build targets depend on this script's outputs.
Report and review by Andres Freund
Discussion: https://www.postgresql.org/message-id/CAFBsxsGW%3DWRbnxXrc8UqqR479XuxtukSFWV-hnmtgsbuNAUO6w%40mail.gmail.com
This commit raises a warning message for a combination of options
('copy_data = true' and 'origin = none') during CREATE/ALTER subscription
operations if the publication tables were also replicated from other
publishers.
During replication, we can skip the data from other origins as we have that
information in WAL but that is not possible during initial sync so we raise
a warning if there is such a possibility.
Author: Vignesh C
Reviewed-By: Peter Smith, Amit Kapila, Jonathan Katz, Shi yu, Wang wei
Discussion: https://www.postgresql.org/message-id/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ@mail.gmail.com
c6e0fe1f2 recently changed the way we store headers for allocated chunks
of memory. Prior to that commit, we stored a pointer to the owning
MemoryContext directly prior to the pointer to the allocated memory.
That's no longer true and c6e0fe1f2 neglected to update
MemoryContextContains() so that it correctly obtains the owning context
with the new method.
A side effect of this change and c6e0fe1f2, in general, is that it's even
less safe than it was previously to pass MemoryContextContains() an
arbitrary pointer which was not allocated by one of our MemoryContexts.
Previously some comments in MemoryContextContains() seemed to indicate
that the worst that could happen by passing an arbitrary pointer would be
a false positive return value. It seems to me that this was a rather
wishful outlook as we subsequently proceeded to subtract sizeof(void *)
from the given pointer and then dereferenced that memory. So it seems
quite likely that we could have segfaulted instead of returning a false
positive. However, it's not impossible that the memory sizeof(void *)
bytes before the pointer could have been owned by the process, but it's
far less likely to work now as obtaining a pointer to the owning
MemoryContext is less direct than before c6e0fe1f2 and will access memory
that's possibly much further away to obtain the owning MemoryContext.
Because of this, I took the liberty of updating the comment to warn
against any future usages of the function and checked the existing core
usages to ensure that we only ever pass in a pointer to memory allocated
by a MemoryContext.
Extension authors updating their code for PG16 who are using
MemoryContextContains should check to ensure that only NULL pointers and
pointers to chunks allocated with a MemoryContext will ever be passed to
MemoryContextContains.
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20220905230949.kb3x2fkpfwtngz43@awork3.anarazel.de
Traditionally, in MEMORY_CONTEXT_CHECKING builds, we only ever marked a
sentinel byte just beyond the requested size if there happened to be
enough space on the chunk to do so. For Slab and Generation context
types, we only rounded the size of the chunk up to the next maxalign
boundary, so it was often not that likely that those would ever have space
for the sentinel given that the majority of allocation requests are going
to be for sizes which are maxaligned. For AllocSet, it was a little
different as smaller allocations are rounded up to the next power-of-2
value rather than the next maxalign boundary, so we're a bit more likely
to have space for the sentinel byte, especially when we get away from tiny
sized allocations such as 8 or 16 bytes.
Here we make more of an effort to allow space so that there is enough room
for the sentinel byte in more cases. This makes it more likely that we'll
detect when buggy code accidentally writes beyond the end of any of its
memory allocations.
Each of the 3 MemoryContext types has been changed as follows:
The Slab allocator will now always set a sentinel byte. Both the current
usages of this MemoryContext type happen to use chunk sizes which were on
the maxalign boundary, so these never used sentinel bytes previously.
For the Generation allocator, we now always ensure there's enough space in
the allocation for a sentinel byte.
For AllocSet, this commit makes an adjustment for allocation sizes which
are greater than allocChunkLimit. We now ensure there is always space for
a sentinel byte. We don't alter the sentinel behavior for request sizes
<= allocChunkLimit. Making way for the sentinel byte for power-of-2
request sizes would require doubling up to the next power of 2. Some
analysis done on the request sizes made during installcheck shows that a
fairly large portion of allocation requests are for power-of-2 sizes. The
amount of additional memory for the sentinel there seems prohibitive, so
we do nothing for those here.
Author: David Rowley
Discussion: https://postgr.es/m/3478405.1661824539@sss.pgh.pa.us
The addition of published column names forgot to filter on attisdropped,
leading to cases where you could see "........pg.dropped.1........"
or the like as a reportedly-published column.
While we're here, rewrite the new subquery to get a more efficient plan
for it.
Hou Zhijie, per report from Jaime Casanova. Back-patch to v15 where
the bug was introduced. (Sadly, this means we need a post-beta4
catversion bump before beta4 has even hit the streets. I see no
good alternative though.)
Discussion: https://postgr.es/m/Yxa1SU4nH2HfN3/i@ahch-to
Here we remove some dead code from CreateTriggerFiringOn() which was
attempting to find the relevant child partition index corresponding to the
given indexOid. As it turned out, thanks to -Wshadow=compatible-local,
this code was buggy as the code which was finding the child indexes
assigned those to a shadowed variable that directly went out of scope.
The code which thought it was looking at the List of child indexes was
always referencing an empty List.
On further investigation, this code is dead. We never call
CreateTriggerFiringOn() passing a valid indexOid in a way that the
function would actually ever execute the code in question. So, for lack
of a way to test if a fix actually works, let's just remove the dead code
instead.
As a reminder, if there is ever a need to resurrect this code, an Assert()
has been added to remind future feature developers that they might need to
write some code to find the corresponding child index.
Reported-by: Justin Pryzby
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/20220819211824.GX26426@telsasoft.com
In a similar effort to f736e188c and 110d81728, fixup various usages of
string functions where a more appropriate function is available and more
fit for purpose.
These changes include:
1. Use cstring_to_text_with_len() instead of cstring_to_text() when
working with a StringInfoData and the length can easily be obtained.
2. Use appendStringInfoString() instead of appendStringInfo() when no
formatting is required.
3. Use pstrdup(...) instead of psprintf("%s", ...)
4. Use pstrdup(...) instead of psprintf(...) (with no formatting)
5. Use appendPQExpBufferChar() instead of appendPQExpBufferStr() when the
length of the string being appended is 1.
6. appendStringInfoChar() instead of appendStringInfo() when no formatting
is required and string is 1 char long.
7. Use appendPQExpBufferStr(b, .) instead of appendPQExpBuffer(b, "%s", .)
8. Don't use pstrdup when it's fine to just point to the string constant.
I (David) did find other cases of #8 but opted to use #4 instead as I
wasn't certain enough that applying #8 was ok (e.g in hba.c)
Author: Ranier Vilela, David Rowley
Discussion: https://postgr.es/m/CAApHDvo2j2+RJBGhNtUz6BxabWWh2Jx16wMUMWKUjv70Ver1vg@mail.gmail.com
Since these macros just cast whatever you give them to the designated
output type, and many normal uses also cast the output type further, a
number of incorrect uses go undiscovered. The fixes in this patch
have been discovered by changing these macros to inline functions,
which is the subject of a future patch.
Reviewed-by: Aleksander Alekseev <aleksander@timescale.com>
Discussion: https://www.postgresql.org/message-id/flat/8528fb7e-0aa2-6b54-85fb-0c0886dbd6ed%40enterprisedb.com
Commit db0d67db2 tweaked sort costing, which however resulted in a
couple plan changes in our regression tests. Most of the new plans were
fine, but partition_aggregate were meant to test parallel plans and the
new plans were serial.
Fix that by lowering parallel_setup_cost to 0, which is enough to switch
to the parallel plan again.
Commit 1349d2790 already made the plans parallel again, but do this
anyway to keep the tests in sync with 15, to make backpatching simpler.
Report and patch by David Rowley.
Author: David Rowley
Reviewed-by: Tomas Vondra
Discussion: https://postgr.es/m/CAApHDvpVFgWzXdtUQkjyOPhNrNvumRi_=ftgS79KeAZ92tnHKQ@mail.gmail.com
XLogPageRead() can retry internally after a pread() system call has
succeeded, in the case of short reads, and page validation failures
while in standby mode (see commit 0668719801). Due to an oversight in
commit 3f1ce973, these cases could leave stale data in the internal
cache of xlogreader.c without marking it invalid. The main defense
against stale cached data on failure to read a page was in the error
handling path of the calling function ReadPageInternal(), but that
wasn't quite enough for errors handled internally by XLogPageRead()'s
retry loop if we then exited with XLREAD_WOULDBLOCK.
1. ReadPageInternal() now marks the cache invalid before calling the
page_read callback, by setting state->readLen to 0. It'll be set to
a non-zero value only after a successful read. It'll stay valid as
long as the caller requests data in the cached range.
2. XLogPageRead() no long performs internal retries while reading
ahead. While such retries should work, the general philosophy is
that we should give up prefetching if anything unusual happens so we
can handle it when recovery catches up, to reduce the complexity of
the system. Let's do that here too.
3. While here, a new function XLogReaderResetError() improves the
separation between xlogrecovery.c and xlogreader.c, where the former
previously clobbered the latter's internal error buffer directly.
The new function makes this more explicit, and also clears a related
flag, without which a standby would needlessly retry in the outer
function.
Thanks to Noah Misch for tracking down the conditions required for a
rare build farm failure in src/bin/pg_ctl/t/003_promote.pl, and
providing a reproducer.
Back-patch to 15.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20220807003627.GA4168930%40rfd.leadboat.com
The planner has to special-case indexes on boolean columns, because
what we need for an indexscan on such a column is a qual of the shape
of "boolvar = pseudoconstant". For plain bool constants, previous
simplification will have reduced this to "boolvar" or "NOT boolvar",
and we have to reverse that if we want to make an indexqual. There is
existing code to do so, but it only fires when the index's opfamily
is BOOL_BTREE_FAM_OID or BOOL_HASH_FAM_OID. Thus extension AMs, or
extension opclasses such as contrib/btree_gin, are out in the cold.
The reason for hard-wiring the set of relevant opfamilies was mostly
to avoid a catalog lookup in a hot code path. We can improve matters
while not taking much of a performance hit by relying on the
hard-wired set when the opfamily OID is visibly built-in, and only
checking the catalogs when dealing with an extension opfamily.
While here, rename IsBooleanOpfamily to IsBuiltinBooleanOpfamily
to remind future users of that macro of its limitations. At some
point we might want to make indxpath.c's improved version of the
test globally accessible, but it's not presently needed elsewhere.
Zongliang Quan and Tom Lane
Discussion: https://postgr.es/m/f293b91d-1d46-d386-b6bb-4b06ff5c667b@yeah.net
Several backend-side loops scanning one or more directories with
ReadDir() (WAL segment recycle/removal in xlog.c, backend-side directory
copy, temporary file removal, configuration file parsing, some logical
decoding logic and some pgtz stuff) already know the type of the entry
being scanned thanks to the dirent structure associated to the entry, on
platforms where we know about DT_REG, DT_DIR and DT_LNK to make the
difference between a regular file, a directory and a symbolic link.
Relying on the direct structure of an entry saves a few system calls to
stat() and lstat() in the loops updated here, shaving some code while on
it. The logic of the code remains the same, calling stat() or lstat()
depending on if it is necessary to look through symlinks.
Authors: Nathan Bossart, Bharath Rupireddy
Reviewed-by: Andres Freund, Thomas Munro, Michael Paquier
Discussion: https://postgr.es/m/CALj2ACV8n-J-f=yiLUOx2=HrQGPSOZM3nWzyQQvLPcccPXxEdg@mail.gmail.com
The sysroot determination is fairly complex and will soon also be needed when
building with meson. Instead of duplicating the logic, move it to a dedicated
shell script invoked both by configure and meson.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/2180a97c-c026-1b6c-cec8-d6e499f97017@enterprisedb.com
The reverts the following and makes some associated cleanups:
commit f79b803dc: Common SQL/JSON clauses
commit f4fb45d15: SQL/JSON constructors
commit 5f0adec25: Make STRING an unreserved_keyword.
commit 33a377608: IS JSON predicate
commit 1a36bc9db: SQL/JSON query functions
commit 606948b05: SQL JSON functions
commit 49082c2cc: RETURNING clause for JSON() and JSON_SCALAR()
commit 4e34747c8: JSON_TABLE
commit fadb48b00: PLAN clauses for JSON_TABLE
commit 2ef6f11b0: Reduce running time of jsonb_sqljson test
commit 14d3f24fa: Further improve jsonb_sqljson parallel test
commit a6baa4bad: Documentation for SQL/JSON features
commit b46bcf7a4: Improve readability of SQL/JSON documentation.
commit 112fdb352: Fix finalization for json_objectagg and friends
commit fcdb35c32: Fix transformJsonBehavior
commit 4cd8717af: Improve a couple of sql/json error messages
commit f7a605f63: Small cleanups in SQL/JSON code
commit 9c3d25e17: Fix JSON_OBJECTAGG uniquefying bug
commit a79153b7a: Claim SQL standard compliance for SQL/JSON features
commit a1e7616d6: Rework SQL/JSON documentation
commit 8d9f9634e: Fix errors in copyfuncs/equalfuncs support for JSON node types.
commit 3c633f32b: Only allow returning string types or bytea from json_serialize
commit 67b26703b: expression eval: Fix EEOP_JSON_CONSTRUCTOR and EEOP_JSONEXPR size.
The release notes are also adjusted.
Backpatch to release 15.
Discussion: https://postgr.es/m/40d2c882-bcac-19a9-754d-4299e1d87ac7@postgresql.org
Not passing -shared to gcc when building a shared library triggers linking to
the wrong libgcc (libgcc.a instead of libgcc_s.a) and prevents emitting
correct unwind information. It's somewhat surprising that this hasn't caused
known problems so far.
Doing so requires adding path to libgcc to libpath, or linking statically to
libgcc - as the latter increases .so size substantially (for not entirely
obvious reasons), shared linking seems preferrable. It likely is worth
building executables with -shared-libgcc too, but I've not done that here.
Discussion: https://postgr.es/m/20220820174213.d574qde4ptwdzoqz@awork3.anarazel.de
Buildfarm member bowerbird is (inconsistently) showing different
results for this test case since we enabled ASLR for MSVC builds.
It's not very clear whether that's a bug in its version of libxml2
or the test case is relying on nominally-undefined behavior, ie the
ordering of results from XPath's node(). It seems quite unlikely
that it's *our* bug though, and what's more, using node() adds
nothing to the test coverage so far as our code is concerned.
So, tweak the test to not use node().
For the moment, only change HEAD because we've only seen the
problem there. Perhaps a case will emerge for back-patching.
Discussion: https://postgr.es/m/2655387.1661695793@sss.pgh.pa.us
During dumptuples() the call to writetuple() would pfree any non-null
tuple. This was quite wasteful as this happens just before we perform a
reset of the context which stores all of those tuples.
It seems to make sense to do a bit of a code refactor to make this work,
so here we just get rid of the writetuple function and adjust the WRITETUP
macro to call the state's writetup function. The WRITETUP usage in
mergeonerun() always has state->slabAllocatorUsed == true, so writetuple()
would never free the tuple or do any memory accounting. The only call
path that needs memory accounting done is in dumptuples(), so let's just
do it manually there.
In passing, let's get rid of the state->memtupcount-- code that counts the
memtupcount down to 0 one tuple at a time inside the loop. That seems to
be a rather inefficient way to set memtupcount to 0, so let's just zero it
after the loop instead.
Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvqZXoDCyrfCzZJR0-xH+7_q+GgitcQiYXUjRani7h4j8Q@mail.gmail.com
get_database_list() failed to restore the caller's memory context,
instead leaving current context set to TopMemoryContext which is
how CommitTransactionCommand() leaves it. The callers both think
they are using short-lived contexts, for the express purpose of
not having to worry about cleaning up individual allocations.
The net effect therefore is that supposedly short-lived allocations
could accumulate indefinitely in the launcher's TopMemoryContext.
Although this has been broken for a long time, it seems we didn't
have any obvious memory leak here until v15's rearrangement of the
stats logic. I (tgl) am not entirely convinced that there's no
other leak at all, though, and we're surely at risk of adding one
in future back-patched fixes. So back-patch to all supported
branches, even though this may be only a latent bug in pre-v15.
Reid Thompson
Discussion: https://postgr.es/m/972a4e12b68b0f96db514777a150ceef7dcd2e0f.camel@crunchydata.com
Before now, the cutoffs that VACUUM used to determine which XIDs/MXIDs
to freeze were determined at the start of each VACUUM by taking related
cutoffs that represent which XIDs/MXIDs VACUUM should treat as still
running, and subtracting an XID/MXID age based value controlled by GUCs
like vacuum_freeze_min_age. The FreezeLimit cutoff (XID freeze cutoff)
was derived by subtracting an XID age value from OldestXmin, while the
MultiXactCutoff cutoff (MXID freeze cutoff) was derived by subtracting
an MXID age value from OldestMxact. This approach didn't match the
approach used nearby to determine whether this VACUUM operation should
be an aggressive VACUUM or not.
VACUUM now uses the standard approach instead: it subtracts the same
age-based values from next XID/next MXID (rather than subtracting from
OldestXmin/OldestMxact). This approach is simpler and more uniform.
Most of the time it will have only a negligible impact on how and when
VACUUM freezes. It will occasionally make VACUUM more robust in the
event of problems caused by long running transaction. These are cases
where OldestXmin and OldestMxact are held back by so much that they
attain an age that is a significant fraction of the value of age-based
settings like vacuum_freeze_min_age.
There is no principled reason why freezing should be affected in any way
by the presence of a long-running transaction -- at least not before the
point that the OldestXmin and OldestMxact limits used by each VACUUM
operation attain an age that makes it unsafe to freeze some of the
XIDs/MXIDs whose age exceeds the value of the relevant age-based
settings. The new approach should at least make freezing degrade more
gracefully than before, even in the most extreme cases.
Author: Peter Geoghegan <pg@bowt.ie>
Reviewed-By: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-By: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/CAH2-WzkOv5CEeyOO=c91XnT5WBR_0gii0Wn5UbZhJ=4TTykDYg@mail.gmail.com
Building the ecpg tests with MSVC, with warnings enabled, results in the
following warning:
src/interfaces/ecpg/test/compat_informix/rnull.pgc(19,1): warning C4305: 'initializing': truncation from 'double' to 'float'
The more obvious fix would be an 'f' suffix, but ecpg can't parse that.
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/2180a97c-c026-1b6c-cec8-d6e499f97017@enterprisedb.com
If the input word exceeds 1000 bytes, don't pass it to the stemmer;
just return it as-is after case folding. Such an input is surely
not a word in any human language, so whatever the stemmer might
do to it would be pretty dubious in the first place. Adding this
restriction protects us against a known recursion-to-stack-overflow
problem in the Turkish stemmer, and it seems like good insurance
against any other safety or performance issues that may exist in
the Snowball stemmers. (I note, for example, that they contain no
CHECK_FOR_INTERRUPTS calls, so we really don't want them running
for a long time.) The threshold of 1000 bytes is arbitrary.
An alternative definition could have been to treat such words as
stopwords, but that seems like a bigger break from the old behavior.
Per report from Egor Chindyaskin and Alexander Lakhin.
Thanks to Olly Betts for the recommendation to fix it this way.
Discussion: https://postgr.es/m/1661334672.728714027@f473.i.mail.ru
Commit e3ce2de09d rearranged this
function to be able to identify which inherited role had admin option
on the target role, but it got the order of operations wrong, causing
the function to return wrong answers in the presence of non-inherited
grants.
Fix that, and add a test case that verifies the correct behavior.
Patch by me, reviewed by Nathan Bossart
Discussion: http://postgr.es/m/CA+TgmoYamnu-xt-u7CqjYWnRiJ6BQaSpYOHXP=r4QGTfd1N_EA@mail.gmail.com
When reporting failure in check_ functions there is (typically) a text-
file mentioned in the error report which contains further details. Some
check_ functions kept a separate flag variable to indicate failure, and
some just checked the state of the filehandle as it's guaranteed to be
open when the check failed. This refactors the functions to consistently
do the same check on error reporting. As the error report contains the
filepath, it makes more sense to check the filehandle state and skip the
flag variable.
Reviewed-by: Nathan Bossart <nathandbossart@gmail.com>
Reviewed-by: Bruce Momjian <bruce@momjian.us>
Discussion: https://postgr.es/m/595759F6-625B-4ED7-8125-91AF00437F83@yesql.se
The default of lazy symbol resolution means that when the postmaster
first reaches the select() call in ServerLoop, it'll need to resolve
the link to that libc entry point. NetBSD's dynamic loader takes
an internal lock while doing that, and if a signal interrupts the
operation then there is a risk of self-deadlock should the signal
handler do anything that requires that lock, as several of the
postmaster signal handlers do. The window for this is pretty narrow,
and timing considerations make it unlikely that a signal would arrive
right then anyway. But it's semi-repeatable on slow single-CPU
machines, and in principle the race could happen with any hardware.
The least messy solution to this is to force binding of dynamic
symbols at postmaster start, using the "-z now" linker option.
While we're at it, also use "-z relro" so as to provide a small
security gain.
It's not entirely clear whether any other platforms share this
issue, but for now we'll assume it's NetBSD-specific. (We might
later try to use "-z now" on more platforms for performance
reasons, but that would not likely be something to back-patch.)
Report and patch by me; the idea to fix it this way is from
Andres Freund.
Discussion: https://postgr.es/m/3384826.1661802235@sss.pgh.pa.us
Robert Haas reported that his older clang compiler didn't like the two
Asserts which were verifying that the given MemoryContextMethodID was <=
MEMORY_CONTEXT_METHODID_MASK when building with
-Wtautological-constant-out-of-range-compare. In my (David's) opinion,
the compiler is wrong to warn about that. Newer versions of clang don't
warn about the out of range enum value, so perhaps this was a bug that has
now been fixed. To keep older clang versions happy, let's just cast the
enum value to int to stop the compiler complaining.
The main reason for the Asserts mentioned above to exist are to inform
future developers which are adding new MemoryContexts if they run out of
bit space in MemoryChunk to store the MemoryContextMethodID. As pointed
out by Tom Lane, it seems wise to also add a comment to the header for
that enum to document the restriction on these enum values.
Additionally, also fix an incorrect usage of UINT64CONST() which was
introduced in c6e0fe1f2.
Author: Robert Haas, David Rowley
Discussion: https://postgr.es/m/CA+TgmoYGG2C7Vbw1cjkQRRBL3zOk8SmhrQnsJgzscX=N9AwPrw@mail.gmail.com
This reverts commit df0f4feef. It turns out the problem which was causing
the 32-bit ARM and PPC animals to fail was due to a MAXALIGN problem in
slab.c. This was fixed by d5ee4db0e. The padding that was added in
df0f4feef would only do anything on machines where uint64 was not aligned
to 8 bytes. The 32-bit machines which were failing are not in that
category, so revert this commit.
Discussion: https://postgr.es/m/3209100.1661787561@sss.pgh.pa.us
Currently, the replication origin tracking of the tablesync worker is
dropped by the apply worker. So, there will be a small lag between the
tablesync worker exit and its origin tracking got removed. In the
meantime, new tablesync workers can be launched and will try to set up
a new origin tracking. This can lead the system to reach max configured
limit (max_replication_slots) even if the user has configured the max
limit considering the number of tablesync workers required in the system.
We decided not to back-patch as this can occur in very narrow
circumstances and users have to option to increase the configured limit by
increasing max_replication_slots.
Reported-by: Hubert Depesz Lubaczewski
Author: Ajin Cherian
Reviwed-by: Masahiko Sawada, Peter Smith, Hou Zhijie, Amit Kapila
Discussion: https://postgr.es/m/20220714115155.GA5439@depesz.com
c6e0fe1f2 added a new pointer field to SlabBlock to make it 4 bytes larger
on 32-bit machines. Prior to that commit, the size of that struct was a
multiple of 8, which meant that MAXALIGN(sizeof(SlabBlock)) was the same
as sizeof(SlabBlock), however, after c6e0fe1f2, due to the addition of the
new pointer field to store a pointer to the owning context, that was no
longer true on builds with sizeof(void *) == 4.
This problem was highlighted by an Assert failure which was checking that
the pointer given to pfree() was MAXALIGNED. Various 32-bit ARM buildfarm
animals were failing. These have MAXIMUM_ALIGNOF of 8. The only 32-bit
testing I'd managed to do on c6e0fe1f2 had been on x86, which has a
MAXIMUM_ALIGNOF of 4, therefore did not exhibit this issue.
Here we define Slab_BLOCKHDRSZ and copy what is being done in aset.c and
generation.c for doing calculations based on the size of the context's
block type. This means that SlabAlloc() will now always return a
MAXALIGNed pointer.
This also fixes an incorrect sentinel_ok() check in SlabCheck() which was
incorrectly checking the wrong sentinel byte. This must have previously
not caused any issues due to the fullChunkSize never being large enough to
store the sentinel byte.
Diagnosed-by: Tomas Vondra, Tom Lane
Author: Tomas Vondra, David Rowley
Discussion: https://postgr.es/m/CAA4eK1%2B1JyW5TiL%3DyV-3Uq1CrfnTyn0Xrk5uArt31Z%3D8rgPhXQ%40mail.gmail.com
All the code and comments cleaned up here is irrelevant since 495ed0e.
Note that this removes an assumption that CreateRestrictedToken() may
not exist, something that could have happened when running under Windows
NT as the code stated. Rather than assuming that it may not exist, this
causes pg_ctl to fail hard if the function cannot be loaded.
Reported-by: Justin Pryzby
Discussion: https://postgr.es/m/20220826112637.GD2342@telsasoft.com
More than twenty years ago (79fcde48b), we hacked the postmaster
to avoid a core-dump on systems that didn't support fflush(NULL).
We've mostly, though not completely, hewed to that rule ever since.
But such systems are surely gone in the wild, so in the spirit of
cleaning out no-longer-needed portability hacks let's get rid of
multiple per-file fflush() calls in favor of using fflush(NULL).
Also, we were fairly inconsistent about whether to fflush() before
popen() and system() calls. While we've received no bug reports
about that, it seems likely that at least some of these call sites
are at risk of odd behavior, such as error messages appearing in
an unexpected order. Rather than expend a lot of brain cells
figuring out which places are at hazard, let's just establish a
uniform coding rule that we should fflush(NULL) before these calls.
A no-op fflush() is surely of trivial cost compared to launching
a sub-process via a shell; while if it's not a no-op then we likely
need it.
Discussion: https://postgr.es/m/2923412.1661722825@sss.pgh.pa.us
When a PostgreSQL instance performing archive recovery but not using
standby mode is promoted, and the last WAL segment that it attempted
to read ended in a partial record, the previous code would create
invalid WAL on the new timeline. The WAL from the previously timeline
would be copied to the new timeline up until the end of the last valid
record, but instead of beginning to write WAL at immediately
afterwards, the promoted server would write an overwrite contrecord at
the beginning of the next segment. The end of the previous segment
would be left as all-zeroes, resulting in failures if anything tried
to read WAL from that file.
The root of the issue is that ReadRecord() decides whether to set
abortedRecPtr and missingContrecPtr based on the value of StandbyMode,
but ReadRecord() switches to a new timeline based on the value of
ArchiveRecoveryRequested. We shouldn't try to write an overwrite
contrecord if we're switching to a new timeline, so change the test in
ReadRecod() to check ArchiveRecoveryRequested instead.
Code fix by Dilip Kumar. Comments by me incorporating suggested
language from Álvaro Herrera. Further review from Kyotaro Horiguchi
and Sami Imseih.
Discussion: http://postgr.es/m/CAFiTN-t7umki=PK8dT1tcPV=mOUe2vNhHML6b3T7W7qqvvajjg@mail.gmail.com
Discussion: http://postgr.es/m/FB0DEA0B-E14E-43A0-811F-C1AE93D00FF3%40amazon.com
Buildfarm animals skate, grison and mamba are Assert failing on the
pointer being given to repalloc not being MAXALIGNED. c6e0fe1f2a made
changes in that area.
All of these animals are 32-bit with a MAXIMUM_ALIGNOF of 8 and a
SIZEOF_VOID_P of 4. I suspect that the pointer is not properly aligned due
to the lack of padding in the MemoryChunk struct.
Here we add the same type of padding that was previously used in
AllocChunkData and GenerationChunk that c6e0fe1f2a neglected to add.
Discussion: https://postgr.es/m/CAA4eK1%2B1JyW5TiL%3DyV-3Uq1CrfnTyn0Xrk5uArt31Z%3D8rgPhXQ%40mail.gmail.com
NEON support is required on the Aarch64 architecture for standard
implementations. Hardware designers for specialized markets can choose
not to support it, but that's true of floating point as well, which
we assume is supported. As with x86, some SIMD support is available
on 32-bit platforms, but those are not interesting from a performance
standpoint and would require an inconvenient runtime check.
Nathan Bossart
Reviewed by John Naylor, Andres Freund, Thomas Munro, and Tom Lane
Discussion: https://www.postgresql.org/message-id/flat/CAFBsxsEyR9JkfbPcDXBRYEfdfC__OkwVGdwEAgY4Rv0cvw35EA%40mail.gmail.com#aba7a64b11503494ffd8dd27067626a9
Whenever we palloc a chunk of memory, traditionally, we prefix the
returned pointer with a pointer to the memory context to which the chunk
belongs. This is required so that we're able to easily determine the
owning context when performing operations such as pfree() and repalloc().
For the AllocSet context, prior to this commit we additionally prefixed
the pointer to the owning context with the size of the chunk. This made
the header 16 bytes in size. This 16-byte overhead was required for all
AllocSet allocations regardless of the allocation size.
For the generation context, the problem was worse; in addition to the
pointer to the owning context and chunk size, we also stored a pointer to
the owning block so that we could track the number of freed chunks on a
block.
The slab allocator had a 16-byte chunk header.
The changes being made here reduce the chunk header size down to just 8
bytes for all 3 of our memory context types. For small to medium sized
allocations, this significantly increases the number of chunks that we can
fit on a given block which results in much more efficient use of memory.
Additionally, this commit completely changes the rule that pointers to
palloc'd memory must be directly prefixed by a pointer to the owning
memory context and instead, we now insist that they're directly prefixed
by an 8-byte value where the least significant 3-bits are set to a value
to indicate which type of memory context the pointer belongs to. Using
those 3 bits as an index (known as MemoryContextMethodID) to a new array
which stores the methods for each memory context type, we're now able to
pass the pointer given to functions such as pfree() and repalloc() to the
function specific to that context implementation to allow them to devise
their own methods of finding the memory context which owns the given
allocated chunk of memory.
The reason we're able to reduce the chunk header down to just 8 bytes is
because of the way we make use of the remaining 61 bits of the required
8-byte chunk header. Here we also implement a general-purpose MemoryChunk
struct which makes use of those 61 remaining bits to allow the storage of
a 30-bit value which the MemoryContext is free to use as it pleases, and
also the number of bytes which must be subtracted from the chunk to get a
reference to the block that the chunk is stored on (also 30 bits). The 1
additional remaining bit is to denote if the chunk is an "external" chunk
or not. External here means that the chunk header does not store the
30-bit value or the block offset. The MemoryContext can use these
external chunks at any time, but must use them if any of the two 30-bit
fields are not large enough for the value(s) that need to be stored in
them. When the chunk is marked as external, it is up to the MemoryContext
to devise its own means to determine the block offset.
Using 3-bits for the MemoryContextMethodID does mean we're limiting
ourselves to only having a maximum of 8 different memory context types.
We could reduce the bit space for the 30-bit value a little to make way
for more than 3 bits, but it seems like it might be better to do that only
if we ever need more than 8 context types. This would only be a problem
if some future memory context type which does not use MemoryChunk really
couldn't give up any of the 61 remaining bits in the chunk header.
With this MemoryChunk, each of our 3 memory context types can quickly
obtain a reference to the block any given chunk is located on. AllocSet
is able to find the context to which the chunk is owned, by first
obtaining a reference to the block by subtracting the block offset as is
stored in the 'hdrmask' field and then referencing the block's 'aset'
field. The Generation context uses the same method, but GenerationBlock
did not have a field pointing back to the owning context, so one is added
by this commit.
In aset.c and generation.c, all allocations larger than allocChunkLimit
are stored on dedicated blocks. When there's just a single chunk on a
block like this, it's easy to find the block from the chunk, we just
subtract the size of the block header from the chunk pointer. The size of
these chunks is also known as we store the endptr on the block, so we can
just subtract the pointer to the allocated memory from that. Because we
can easily find the owning block and the size of the chunk for these
dedicated blocks, we just always use external chunks for allocation sizes
larger than allocChunkLimit. For generation.c, this sidesteps the problem
of non-external MemoryChunks being unable to represent chunk sizes >= 1GB.
This is less of a problem for aset.c as we store the free list index in
the MemoryChunk's spare 30-bit field (the value of which will never be
close to using all 30-bits). We can easily reverse engineer the chunk size
from this when needed. Storing this saves AllocSetFree() from having to
make a call to AllocSetFreeIndex() to determine which free list to put the
newly freed chunk on.
For the slab allocator, this commit adds a new restriction that slab
chunks cannot be >= 1GB in size. If there happened to be any users of
slab.c which used chunk sizes this large, they really should be using
AllocSet instead.
Here we also add a restriction that normal non-dedicated blocks cannot be
1GB or larger. It's now not possible to pass a 'maxBlockSize' >= 1GB
during the creation of an AllocSet or Generation context. Allocations can
still be larger than 1GB, it's just these will always be on dedicated
blocks (which do not have the 1GB restriction).
Author: Andres Freund, David Rowley
Discussion: https://postgr.es/m/CAApHDvpjauCRXcgcaL6+e3eqecEHoeRm9D-kcbuvBitgPnW=vw@mail.gmail.com
It has been incorrectly assumed in commit 7f13ac8123 that we can either
purge all or none in the catalog modifying xids list retrieved from a
serialized snapshot. It is quite possible that some of the xids in that
array are old enough to be pruned but not others.
As per buildfarm
Author: Amit Kapila and Masahiko Sawada
Reviwed-by: Masahiko Sawada
Discussion: https://postgr.es/m/CAA4eK1LBtv6ayE+TvCcPmC-xse=DVg=SmbyQD1nv_AaqcpUJEg@mail.gmail.com
This has as effect to add /DYNAMICBASE to the .dll and .exe files
generated by the builds, undoing 7f3e17b. Note that ASLR was already
enabled in MinGW as we have never added --disable-dynamicbase there.
This change will ease a bit the integration of arm64 with MSVC, as ASLR
support is mandatory in this case. So, thanks to this commit, we have
no need to make ASLR conditional depending on the architecture used for
the build.
Andres Freund has done a lot of testing with this option while working
on meson, without seeing /DYNAMICBASE as being a problem in the Windows
builds of the CI. Personally, not supporting anything older than
Windows 10 on HEAD makes me feel safer about this change, as we have
seen ASLR with being a problem in process invocation particularly with
Windows 8 and server 2012 back in 2014, even if Windows 10 was not
really a thing back then. 45e004f is also something that can help in
making the process invocation more stable. We are very early in the
development of Postgres 16, giving a lot of room to detect stability
issues if any.
Discussion: https://postgr.es/m/20220826012907.gjw3jdqdgsts5y65@awork3.anarazel.de
While the bug I just fixed in the back branches doesn't exist in
HEAD, the requirement that MULTIEXPR SubPlans not share output
parameters still does. Add a comment to memorialize that, because
perhaps it could be an issue again someday.
Discussion: https://postgr.es/m/17596-c5357f61427a81dc@postgresql.org
Commit 121d2d3d70 included simd.h into pg_wchar.h. This caused a problem
on Windows, since Perl has "#define free" (referring to globals), which
breaks the Windows' header. To fix, move the static inline function
definitions from plperl_helpers.h, into plperl.h, where we already
document the necessary inclusion order. Since those functions were the
only reason for the existence of plperl_helpers.h, remove it.
First reported by Justin Pryzby
Diagnosis and review by Andres Freund, patch by myself per suggestion
from Tom Lane
Discussion: https://www.postgresql.org/message-id/20220826115546.GE2342%40telsasoft.com
While waiting for slots to become available in wait_on_slots() in
parallel_slot.c, the cancellation always relied on the first connection
in the set to do the job. This could cause problems when this slot's
socket is gone as PQgetCancel() would return NULL in this case. Rather
than always using the first connection, this changes the logic to use
the first valid connection for the cancellation.
Author: Ranier Vilela
Reviewed-by: Justin Pryzby
Discussion: https://postgr.es/m/CAEudQAokk1h_pUwGXsYS4oVOuf35s1O2o3TXGHpV8=AWikvgHA@mail.gmail.com
Backpatch-through: 14
Per flame graph from Jelte Fennema, COPY FROM ... USING BINARY shows
input validation taking at least 5% of the profile, so it's worth trying
to be more efficient here. With this change, validation of pure ASCII is
nearly 40% faster on contemporary Intel hardware. To make this change
legible and easier to adopt to additional architectures, use helper
functions to abstract the platform details away.
Reviewed by Nathan Bossart
Discussion: https://www.postgresql.org/message-id/CAFBsxsG%3Dk8t%3DC457FXnoBXb%3D8iA4OaZkbFogFMachWif7mNnww%40mail.gmail.com
The comment in basebackup.c updated by 33bd4698c1 was actually
obsolete to begin with, since the symbols it was referring to haven't
existed in that header file for quite some time. The header file is
still needed for other reasons, though, so keep the #include, just
drop the comment.
SUSv3 <netinet/in.h> defines struct sockaddr_in6, and all targeted Unix
systems have it. Windows has it in <ws2ipdef.h>. Remove the configure
probe, the macro and a small amount of dead code.
Also remove a mention of IPv6-less builds from the documentation, since
there aren't any.
This is similar to commits f5580882 and 077bf2f2 for Unix sockets. Even
though AF_INET6 is an "optional" component of SUSv3, there are no known
modern operating system without it, and it seems even less likely to be
omitted from future systems than AF_UNIX.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA+hUKGKErNfhmvb_H0UprEmp4LPzGN06yR2_0tYikjzB-2ECMw@mail.gmail.com
In a similar effort to f01592f91, here we're targetting fixing the
warnings where we've deemed the shadowing variable to serve a close enough
purpose to the shadowed variable just to reuse the shadowed version and
not declare the shadowing variable at all.
By my count, this takes the warning count from 106 down to 71.
Author: Justin Pryzby
Discussion: https://postgr.es/m/20220825020839.GT2342@telsasoft.com
The GRANT statement can now specify WITH INHERIT TRUE or WITH
INHERIT FALSE to control whether the member inherits the granted
role's permissions. For symmetry, you can now likewise write
WITH ADMIN TRUE or WITH ADMIN FALSE to turn ADMIN OPTION on or off.
If a GRANT does not specify WITH INHERIT, the behavior based on
whether the member role is marked INHERIT or NOINHERIT. This means
that if all roles are marked INHERIT or NOINHERIT before any role
grants are performed, the behavior is identical to what we had before;
otherwise, it's different, because ALTER ROLE [NO]INHERIT now only
changes the default behavior of future grants, and has no effect on
existing ones.
Patch by me. Reviewed and testing by Nathan Bossart and Tushar Ahuja,
with design-level comments from various others.
Discussion: http://postgr.es/m/CA+Tgmoa5Sf4PiWrfxA=sGzDKg0Ojo3dADw=wAHOhR9dggV=RmQ@mail.gmail.com
The dependencies here aren't quite right independent of vpath builds or not,
but this at least makes vpath builds succeed. And it's pretty rare to change
the exports.txt file anyway... The referenced thread has a patch that will
clean that up further.
Discussion: https://postgr.es/m/20220820174213.d574qde4ptwdzoqz@awork3.anarazel.de
This is preparatory work for a project to increase the number of bits
in a RelFileNumber from 32 to 56.
Along the way, introduce static inline accessor functions for a couple
of BufferTag fields.
Dilip Kumar, reviewed by me. The overall patch series has also had
review at various times from Andres Freund, Ashutosh Sharma, Hannu
Krosing, Vignesh C, Álvaro Herrera, and Tom Lane.
Discussion: http://postgr.es/m/CAFiTN-trubju5YbWAq-BSpZ90-Z6xCVBQE8BVqXqANOZAF1Znw@mail.gmail.com
SplitToVariants() in the ispell code, lseg_inside_poly() in geo_ops.c,
and regex_selectivity_sub() in selectivity estimation could recurse
until stack overflow; fix by adding check_stack_depth() calls.
So could next() in the regex compiler, but that case is better fixed by
converting its tail recursion to a loop. (We probably get better code
that way too, since next() can now be inlined into its sole caller.)
There remains a reachable stack overrun in the Turkish stemmer, but
we'll need some advice from the Snowball people about how to fix that.
Per report from Egor Chindyaskin and Alexander Lakhin. These mistakes
are old, so back-patch to all supported branches.
Richard Guo and Tom Lane
Discussion: https://postgr.es/m/1661334672.728714027@f473.i.mail.ru
These should have been included in 421892a19 as these shadowed variable
warnings can also be fixed by adjusting the scope of the shadowed variable
to put the declaration for it in an inner scope.
This is part of the same effort as f01592f91.
By my count, this takes the warning count from 114 down to 106.
Author: David Rowley and Justin Pryzby
Discussion: https://postgr.es/m/CAApHDvrwLGBP%2BYw9vriayyf%3DXR4uPWP5jr6cQhP9au_kaDUhbA%40mail.gmail.com
It is not customary to install a shared library with a minor version
number (libpq.5.16.dylib) on macOS. We just need the file with the
major version number (libpq.5.dylib) and the one without version
number (libpq.dylib). This also matches the installation layout used
by Meson.
Discussion: https://www.postgresql.org/message-id/e0c44fb2-8b66-a4b9-b274-7ed3a1a0ab74@enterprisedb.com
This commit moves authn_id into a new global structure called
ClientConnectionInfo (mapping to a MyClientConnectionInfo for each
backend) which is intended to hold all the client information that
should be shared between the backend and any of its parallel workers,
access for extensions and triggers being the primary use case. There is
no need to push all the data of Port to the workers, and authn_id is
quite a generic concept so using a separate structure provides the best
balance (the name of the structure has been suggested by Robert Haas).
While on it, and per discussion as this would be useful for a potential
SYSTEM_USER that can be accessed through parallel workers, a second
field is added for the authentication method, copied directly from
Port.
ClientConnectionInfo is serialized and restored using a new parallel
key and a structure tracks the length of the authn_id, making the
addition of more fields straight-forward.
Author: Jacob Champion
Reviewed-by: Bertrand Drouvot, Stephen Frost, Robert Haas, Tom Lane,
Michael Paquier, Julien Rouhaud
Discussion: https://postgr.es/m/793d990837ae5c06a558d58d62de9378ab525d83.camel@vmware.com
In a similar effort to f01592f91, here we're targetting fixing the
warnings that -Wshadow=compatible-local produces that we can fix by moving
a variable to an inner scope to stop that variable from being shadowed by
another variable declared somewhere later in the function.
All of the warnings being fixed here are changing the scope of variables
which are being used as an iterator for a "for" loop. In each instance,
the fix happens to be changing the for loop to use the C99 type
initialization. Much of this code likely pre-dates our use of C99.
Reducing the scope of the outer scoped variable seems like the safest way
to fix these. Renaming seems more likely to risk patches using the wrong
variable. Reducing the scope is more likely to result in a compilation
failure after applying some future patch rather than introducing bugs with
it.
By my count, this takes the warning count from 129 down to 114.
Author: Justin Pryzby
Discussion: https://postgr.es/m/CAApHDvrwLGBP%2BYw9vriayyf%3DXR4uPWP5jr6cQhP9au_kaDUhbA%40mail.gmail.com
I added this in commit 153f40067, out of paranoia about kernels
possibly rejecting very large listen backlog requests. However,
POSIX has said for decades that the kernel must silently reduce
any value it considers too large, and there's no evidence that
any current system doesn't obey that. Let's just drop this limit
and save some complication.
While we're here, compute the request as twice MaxConnections not
twice MaxBackends; the latter no longer means what it did in 2001.
Per discussion of a report from Kevin McKibbin.
Discussion: https://postgr.es/m/CADc_NKg2d+oZY9mg4DdQdoUcGzN2kOYXBu-3--RW_hEe0tUV=g@mail.gmail.com
sysctl is more portable than Linux's /proc/sys file tree, and
often easier to use too. That's why most of our docs refer to
sysctl when talking about how to adjust kernel parameters.
Bring the few stragglers into line.
Discussion: https://postgr.es/m/361175.1661187463@sss.pgh.pa.us
All backends should have a BackendType to enable statistics reporting
per BackendType.
Add a new BackendType for standalone backends, B_STANDALONE_BACKEND (and
alphabetize the BackendTypes). Both the bootstrap backend and single
user mode backends will have BackendType B_STANDALONE_BACKEND.
Author: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/CAAKRu_aaq33UnG4TXq3S-OSXGWj1QGf0sU%2BECH4tNwGFNERkZA%40mail.gmail.com
Somewhere during the development of the patch acquiring a lock during read
access to variable-numbered stats got lost. The missing lock acquisition won't
cause corruption, but can lead to reading torn values when accessing
stats. Add the missing lock acquisitions.
Reported-by: Greg Stark <stark@mit.edu>
Reviewed-by: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CAM-w4HMYkM_DkYhWtUGV+qE_rrBxKOzOF0+5faozxO3vXrc9wA@mail.gmail.com
Backpatch: 15-
Previously, membership of role A in role B could be recorded in the
catalog tables only once. This meant that a new grant of role A to
role B would overwrite the previous grant. For other object types, a
new grant of permission on an object - in this case role A - exists
along side the existing grant provided that the grantor is different.
Either grant can be revoked independently of the other, and
permissions remain so long as at least one grant remains. Make role
grants work similarly.
Previously, when granting membership in a role, the superuser could
specify any role whatsoever as the grantor, but for other object types,
the grantor of record must be either the owner of the object, or a
role that currently has privileges to perform a similar GRANT.
Implement the same scheme for role grants, treating the bootstrap
superuser as the role owner since roles do not have owners. This means
that attempting to revoke a grant, or admin option on a grant, can now
fail if there are dependent privileges, and that CASCADE can be used
to revoke these. It also means that you can't grant ADMIN OPTION on
a role back to a user who granted it directly or indirectly to you,
similar to how you can't give WITH GRANT OPTION on a privilege back
to a role which granted it directly or indirectly to you.
Previously, only the superuser could specify GRANTED BY with a user
other than the current user. Relax that rule to allow the grantor
to be any role whose privileges the current user posseses. This
doesn't improve compatibility with what we do for other object types,
where support for GRANTED BY is entirely vestigial, but it makes this
feature more usable and seems to make sense to change at the same time
we're changing related behaviors.
Along the way, fix "ALTER GROUP group_name ADD USER user_name" to
require the same privileges as "GRANT group_name TO user_name".
Previously, CREATEROLE privileges were sufficient for either, but
only the former form was permissible with ADMIN OPTION on the role.
Now, either CREATEROLE or ADMIN OPTION on the role suffices for
either spelling.
Patch by me, reviewed by Stephen Frost.
Discussion: http://postgr.es/m/CA+TgmoaFr-RZeQ+WoQ5nKPv97oT9+aDgK_a5+qWHSgbDsMp1Vg@mail.gmail.com
Remove four probes for members of sockaddr_storage. Keep only the probe
for sockaddr's sa_len, which is enough for our two remaining places that
know about _len fields:
1. ifaddr.c needs to know if sockaddr has sa_len to understand the
result of ioctl(SIOCGIFCONF). Only AIX is still using the relevant code
today, but it seems like a good idea to keep it compilable on Linux.
2. ip.c was testing for presence of ss_len to decide whether to fill in
sun_len in our getaddrinfo_unix() function. It's just as good to test
for sa_len. If you have one, you have them all.
(The code in #2 isn't actually needed at all on several OSes I checked
since modern versions ignore sa_len on input to system calls. Proving
that's the case for all relevant OSes is left for another day, but
wouldn't get rid of that last probe anyway if we still want it for #1.)
Discussion: https://postgr.es/m/CA%2BhUKGJJjF2AqdU_Aug5n2MAc1gr%3DGykNjVBZq%2Bd6Jrcp3Dyvg%40mail.gmail.com
As such the current usage of & won't produce incorrect results but it
would be better to use && to short-circuit the evaluation of second
condition when the same is not required.
Author: Ranier Vilela
Reviewed-by: Tom Lane, Bharath Rupireddy
Backpatch-through: 15, where it was introduced
Discussion: https://postgr.es/m/CAEudQApL8QcoYwQuutkWKY_h7gBY8F0Xs34YKfc7-G0i83K_pw@mail.gmail.com
The ecpg tests have their input directory in the build directory, as the tests
need to be built. Until now that required copying the expected/ directory to
the build directory in VPATH builds. To avoid needing to implement the same
for the meson build, add support for specifying the location of the expected
directory.
Now that that's not needed anymore, remove the copying of ecpg's expected
directory to the build directory in VPATH builds.
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Discussion: https://postgr.es/m/20220718202327.pspcqz5mwbi2yb7w@awork3.anarazel.de
Compiling with -Wshadow=compatible-local yields quite a few warnings about
local variables being shadowed by compatible local variables in an inner
scope. Of course, this is perfectly valid in C, but we have had bugs in
the past as a result of developers failing to notice this. af7d270dd is a
recent example.
Here we do a cleanup of warnings we receive from -Wshadow=compatible-local
for code which is new to PostgreSQL 15. We've yet to have the discussion
about if we actually ever want to run that as a standard compilation flag.
We'll need to at least get the number of warnings down to something easier
to manage before we can realistically consider if we want this or not.
This commit is the first step towards reducing the warnings.
The changes being made here are all fairly trivial. Because of that, and
the fact that v15 is still in beta, this is being back-patched into 15.
It seems more risky not to do this as the risk of future bugs is increased
by the additional conflicts that this commit could cause for any future
bug fixes touching the same areas as this commit.
Author: Justin Pryzby
Discussion: https://postgr.es/m/20220817145434.GC26426%40telsasoft.com
Backpatch-through: 15
Consistently avoid trusting a sample of only one page at the point that
VACUUM determines a new reltuples for the target table (though only when
the table is larger than a single page). This is follow-up work to
commit 74388a1a, which added a heuristic to prevent reltuples from
becoming distorted by successive VACUUM operations that each scan only a
single heap page (which was itself more or less a bugfix for an issue in
commit 44fa8488, which simplified VACUUM's handling of scanned pages).
The original bugfix commit did not account for certain remaining cases
that where not affected by its "2% of total relpages" heuristic. This
happened with relations that are small enough that just one of its pages
exceeded the 2% threshold, yet still big enough for VACUUM to deem
skipping most of its pages via the visibility map worthwhile. reltuples
could still become distorted over time with such a table, at least in
scenarios where the VACUUM command is run repeatedly and without the
table itself ever changing.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-Wzk7d4m3oEbEWkWQKd+gz-eD_peBvdXVk1a_KBygXadFeg@mail.gmail.com
Backpatch: 15-, where the rules for scanned pages changed.
Initialize shared memory allocated for index stats to avoid a hard
crash. This was possible when parallel VACUUM became confused about the
current phase of index processing.
Oversight in commit 8e1fae1938, which refactored parallel VACUUM.
Author: Masahiko Sawada <sawada.mshk@gmail.com>
Reported-By: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/20220818133406.GL26426@telsasoft.com
Backpatch: 15-, the first version with the refactoring commit.
Previously, "GRANT foo TO bar" or "GRANT foo TO bar GRANTED BY baz"
would record the OID of the grantor in pg_auth_members.grantor, but
that role could later be dropped without modifying or removing the
pg_auth_members record. That's not great, because we typically try
to avoid dangling references in catalog data.
Now, a role grant depends on the grantor, and the grantor can't be
dropped without removing the grant or changing the grantor. "DROP
OWNED BY" will remove the grant, just as it does for other kinds of
privileges. "REASSIGN OWNED BY" will not, again just like what we do
in other cases involving privileges.
pg_auth_members now has an OID column, because that is needed in order
for dependencies to work. It also now has an index on the grantor
column, because otherwise dropping a role would require a sequential
scan of the entire table to see whether the role's OID is in use as
a grantor. That probably wouldn't be too large a problem in practice,
but it seems better to have an index just in case.
A follow-on patch is planned with the goal of more thoroughly
rationalizing the behavior of role grants. This patch is just trying
to do enough to make sure that the data we store in the catalogs is at
some basic level valid.
Patch by me, reviewed by Stephen Frost
Discussion: http://postgr.es/m/CA+TgmoaFr-RZeQ+WoQ5nKPv97oT9+aDgK_a5+qWHSgbDsMp1Vg@mail.gmail.com
The present implementations of adjust_appendrel_attrs_multilevel and
its sibling adjust_child_relids_multilevel are very messy, because
they work by reconstructing the relids of the child's immediate
parent and then seeing if that's bms_equal to the relids of the
target parent. Aside from being quite inefficient, this will not
work with planned future changes to make joinrels' relid sets
contain outer-join relids in addition to baserels.
The whole thing can be solved at a stroke by adding explicit parent
and top_parent links to child RelOptInfos, and making these functions
work with RelOptInfo pointers instead of relids. Doing that is
simpler for most callers, too.
In my original version of this patch, I got rid of
RelOptInfo.top_parent_relids on the grounds that it was now redundant.
However, that adds a lot of code churn in places that otherwise would
not need changing, and arguably the extra indirection needed to fetch
top_parent->relids in those places costs something. So this version
leaves that field in place.
Discussion: https://postgr.es/m/553080.1657481916@sss.pgh.pa.us
As written, if you use XLogBeginRead() to position an xlogreader at
the beginning of a WAL page and then try to read WAL, this assertion
will fail. However, the header comment for XLogBeginRead() claims
that positioning an xlogreader at the beginning of a page is valid,
and the code here is perfectly able to cope with it. It's only the
assertion that causes trouble. So relax it.
This is formally a bug in all supported branches, but as it doesn't
seem to have any consequences for current uses of the xlogreader
facility, no back-patch, at least for now.
Dilip Kumar and Robert Haas
Discussion: http://postgr.es/m/CA+TgmoaJSs2_7WHW2GzFYe9+zfPtxBKvT3GW47+x=ptUE=cULw@mail.gmail.com
When creating a partitioned index, DefineIndex tries to identify
any existing indexes on the partitions that match the partitioned
index, so that it can absorb those as child indexes instead of
building new ones. Part of the matching is to compare IndexInfo
structs --- but that wasn't done quite right. We're comparing
the IndexInfo built within DefineIndex itself to one made from
existing catalog contents by BuildIndexInfo. Notably, while
BuildIndexInfo will run index expressions and predicates through
expression preprocessing, that has not happened to DefineIndex's
struct. The result is failure to match and subsequent creation
of duplicate indexes.
The easiest and most bulletproof fix is to build a new IndexInfo
using BuildIndexInfo, thereby guaranteeing that the processing done
is identical.
While here, let's also extract the opfamily and collation data
from the new partitioned index, removing ad-hoc logic that
duplicated knowledge about how those are constructed.
Per report from Christophe Pettus. Back-patch to v11 where
we invented partitioned indexes.
Richard Guo and Tom Lane
Discussion: https://postgr.es/m/8864BFAA-81FD-4BF9-8E06-7DEB8D4164ED@thebuild.com
configure extracts TCL_SHLIB_LD_LIBS from tclConfig.sh, and puts the
value into Makefile.global, but then we never use it anywhere. It
looks like I removed the only usage in cd75f94da, but didn't notice
that it was the only usage. Might as well mop this up while we're
trying to get rid of unnecessary configure steps.
Discussion: https://postgr.es/m/2442359.1660835043@sss.pgh.pa.us
Commit 5579388d was confused about why gai_strerror() didn't work, and
used gai_strerrorA(). It turns out that we had explicitly undefined
Windows' own macro for that somewhere else. Get rid of all that, and
use the system headers' definition of gai_sterror() directly as
intended.
Discussion: https://postgr.es/m/CA+hUKGKErNfhmvb_H0UprEmp4LPzGN06yR2_0tYikjzB-2ECMw@mail.gmail.com
On BSD-family systems, header <sys/sockio.h> defines socket ioctl
numbers like SIOCGIFCONF. Only AIX is using those now, but it defines
them in <net/if.h> anyway.
Supposing some PostgreSQL hacker wants to test that AIX-only code path
on a more common development system by pretending not to have
getifaddrs(). It's enough to include <sys/ioctl.h>, at least on macOS,
FreeBSD and Linux, and we're already doing that.
We carried a special implementation of pg_foreach_ifaddr() using
Solaris's ioctl(SIOCGLIFCONF), but Solaris 11 and illumos adopted
getifaddrs() more than a decade ago, and we prefer to use that. Solaris
10 is EOL'd. Remove the dead code.
Adjust comment about which OSes have getifaddrs(), which also
incorrectly listed AIX. AIX is in fact the only Unix in the build farm
that *doesn't* have it today, so the implementation based on
ioctl(SIOCGIFCONF) (note, no 'L') is still live. All the others have
had it for at least one but mostly two decades.
The last-stop fallback at the bottom of the file is dead code in
practice, but it's hard to justify removing it because the better
options are all non-standard.
Discussion: https://postgr.es/m/CA+hUKGKErNfhmvb_H0UprEmp4LPzGN06yR2_0tYikjzB-2ECMw@mail.gmail.com
The table column that stores this is of type oid, but is actually limited
to uint16 and has a different path for creating new values. Some of
the documentation already referred to it as an ID, so let's standardize
on that.
While at it, most format strings already use %u, so for consintency
change the remaining stragglers using %d.
Per suggestions from Tom Lane and Justin Pryzby
Discussion: https://www.postgresql.org/message-id/3437166.1659620465%40sss.pgh.pa.us
Backpatch to v15
1349d2790 changed things to make the planner request that the
query_pathkeys contain pathkeys for any ORDER BY / DISTINCT aggregates.
Some code added prior to that commit in db0d67db2 made it so the order
that the pathkeys appear in the group_pathkeys could be changed so that
the GROUP BY could be executed in a more optimal order which minimized
sort comparisons. 1349d2790 had to make sure that the pathkeys for any
ORDER BY / DISTINCT aggregates remained at the end of the groupby_pathkeys
and wasn't reordered, so some code was added to
add_paths_to_grouping_rel() to first strip off any pathkeys belonging to
ORDER BY / DISTINCT aggregates before passing to the function to optimize
the order of the group_pathkeys.
It seems I dropped the ball in 1349d2790 and mistakenly used the untouched
PlannerInfo.group_pathkeys to pass to get_useful_group_keys_orderings()
instead of the version that had the aggregate pathkeys removed. It was
only the code path that was handling creating paths for
partially_grouped_rel which made this mistake. In practice, we'll never
have any extra pathkeys to strip off when processing
partially_grouped_rel as that's only used when considering partial
paths, which we never do when there are ORDER BY / DISTINCT aggregates.
So this is just a hypothetical bug, not a live bug. We already have the
correct pathkeys determined, so it's of no extra cost to pass the
correct variable.
Reported-by: Justin Pryzby
Discussion: https://postgr.es/m/20220817015755.GB26426@telsasoft.com
Make build_joinrel_tlist() responsible for adding PHVs that were
already computed in one or the other input relation, and therefore
change add_placeholders_to_joinrel() to only add PHVs that will be
newly computed in this joinrel's output. This makes the handling
of PHVs in build_joinrel_tlist() more like its handling of plain
Vars, which seems like a good thing on intelligibility grounds
and will simplify planned future changes. There is a purely
cosmetic side-effect that the order of entries in the joinrel's
tlist may change; but since it becomes more like the order of
entries in the input tlists, that's not bad.
The reason it wasn't done like this originally was the potential
cost of looking up PlaceHolderInfo entries to consult ph_needed.
Now that that's O(1) it shouldn't hurt.
Discussion: https://postgr.es/m/1405792.1660677844@sss.pgh.pa.us
Up to now, callers of find_placeholder_info() were required to pass
a flag indicating if it's OK to make a new PlaceHolderInfo. That'd
be fine if the callers had free choice, but they do not. Once we
begin deconstruct_jointree() it's no longer OK to make more PHIs;
while callers before that always want to create a PHI if it's not
there already. So there's no freedom of action, only the opportunity
to cause bugs by creating PHIs too late. Let's get rid of that in
favor of adding a state flag PlannerInfo.placeholdersFrozen, which
we can set at the point where it's no longer OK to make more PHIs.
This patch also simplifies a couple of call sites that were using
complicated logic to avoid calling find_placeholder_info() as much
as possible. Now that that lookup is O(1) thanks to the previous
commit, the extra bitmap manipulations are probably a net negative.
Discussion: https://postgr.es/m/1405792.1660677844@sss.pgh.pa.us
Up to now we've just searched the placeholder_list when we want to
find the PlaceHolderInfo with a given ID. While there's no evidence
of that being a problem in the field, an upcoming patch will add
find_placeholder_info() calls in build_joinrel_tlist(), which seems
likely to make it more of an issue: a joinrel emitting lots of
PlaceHolderVars would incur O(N^2) cost, and we might be building
a lot of joinrels in complex queries. Hence, add an array that
can be indexed directly by phid to make the lookups constant-time.
Discussion: https://postgr.es/m/1405792.1660677844@sss.pgh.pa.us
The standard way to check for list emptiness is to compare the
List pointer to NIL; our list code goes out of its way to ensure
that that is the only representation of an empty list. (An
acceptable alternative is a plain boolean test for non-null
pointer, but explicit mention of NIL is usually preferable.)
Various places didn't get that memo and expressed the condition
with list_length(), which might not be so bad except that there
were such a variety of ways to check it exactly: equal to zero,
less than or equal to zero, less than one, yadda yadda. In the
name of code readability, let's standardize all those spellings
as "list == NIL" or "list != NIL". (There's probably some
microscopic efficiency gain too, though few of these look to be
at all performance-critical.)
A very small number of cases were left as-is because they seemed
more consistent with other adjacent list_length tests that way.
Peter Smith, with bikeshedding from a number of us
Discussion: https://postgr.es/m/CAHut+PtQYe+ENX5KrONMfugf0q6NHg4hR5dAhqEXEc2eefFeig@mail.gmail.com
This event can happen when using SET ACCESS METHOD, as the data files of
the materialized need a full refresh but this command tag was not
updated to reflect that. The documentation is updated to track this
behavior.
Author: Onder Kalaci
Discussion: https://postgr.es/m/CACawEhXwHN3X34FiwoYG8vXR-oyUdrp7qcfRWSzS+NPahS5gSw@mail.gmail.com
Backpatch-through: 15
The assert, introduced by 9f1cf97bb5, is intended to check if the prefix
is terminated by a \0 byte, but it has two flaws. Firstly, prefix_size
includes the \0 byte, so prefix[prefix_size] points to the byte after
the null byte. Secondly, the check ensures the byte is not equal \0,
while it should be checking the opposite.
Backpatch-through: 14
Discussion: https://postgr.es/m/b99b6101-2f14-3796-3dfa-4a6cd7d4326d@enterprisedb.com
The current publisher code checks if UPDATE or DELETE can be executed with
the replica identity of the table even if it's a partitioned table. We can
skip checking the replica identity for partitioned tables because the
operations are actually performed on the leaf partitions (not the
partitioned table).
Reported-by: Brad Nicholson
Author: Hou Zhijie
Reviewed-by: Peter Smith, Amit Kapila
Backpatch-through: 13
Discussion: https://postgr.es/m/CAMMnM%3D8i5DohH%3DYKzV0_wYuYSYvuOJoL9F5nzXTc%2ByzsG1f6rg%40mail.gmail.com
There's a convention that externally-visible libpq functions should
check for a NULL PGconn pointer, and fail gracefully instead of
crashing. PQflush() and PQisnonblocking() didn't get that memo
though. Also add a similar check to PQdefaultSSLKeyPassHook_OpenSSL;
while it's not clear that ordinary usage could reach that with a
null conn pointer, it's cheap enough to check, so let's be consistent.
Daniele Varrazzo and Tom Lane
Discussion: https://postgr.es/m/CA+mi_8Zm_mVVyW1iNFgyMd9Oh0Nv8-F+7Y3-BqwMgTMHuo_h2Q@mail.gmail.com
Since WRITE_NODE_FIELD() output always starts with a space, we don't
need to go out of our way to print another space right before it.
This change is only for visual appearance; the tokenizer on the
reading side would read it the same way (but there is no read support
for A_Expr at this time anyway).
When enlarging the work buffers of a VarStringSortSupport object,
varstrfastcmp_locale was careful to keep them in the ssup_cxt
memory context; but varstr_abbrev_convert just used palloc().
The latter creates a hazard that the buffers could be freed out
from under the VarStringSortSupport object, resulting in stomping
on whatever gets allocated in that memory later.
In practice, because we only use this code for ICU collations
(cf. 3df9c374e), the problem is confined to use of ICU collations.
I believe it may have been unreachable before the introduction
of incremental sort, too, as traditional sorting usually just
uses one context for the duration of the sort.
We could fix this by making the broken stanzas in varstr_abbrev_convert
match the non-broken ones in varstrfastcmp_locale. However, it seems
like a better idea to dodge the issue altogether by replacing the
pfree-and-allocate-anew coding with repalloc, which automatically
preserves the chunk's memory context. This fix does add a few cycles
because repalloc will copy the chunk's content, which the existing
coding assumes is useless. However, we don't expect that these buffer
enlargement operations are performance-critical. Besides that, it's
far from obvious that copying the buffer contents isn't required, since
these stanzas make no effort to mark the buffers invalid by resetting
last_returned, cache_blob, etc. That seems to be safe upon examination,
but it's fragile and could easily get broken in future, which wouldn't
get revealed in testing with short-to-moderate-size strings.
Per bug #17584 from James Inform. Whether or not the issue is
reachable in the older branches, this code has been broken on its
own terms from its introduction, so patch all the way back.
Discussion: https://postgr.es/m/17584-95c79b4a7d771f44@postgresql.org
SUSv3, all targeted Unixes and modern Windows have getaddrinfo() and
related interfaces. Drop the replacement implementation, and adjust
some headers slightly to make sure that the APIs are visible everywhere
using standard POSIX headers and names.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CA%2BhUKG%2BL_3brvh%3D8e0BW_VfX9h7MtwgN%3DnFHP5o7X2oZucY9dg%40mail.gmail.com
