This is in the same spirit as ef5e2e9085, updating this time some
arrays in parser.c, relpath.c, guc_tables.c and pg_dump_sort.c so as the
order of their elements has no need to match the enum structures they
are based on anymore.
Author: Jelte Fennema-Nio
Reviewed-by: Jian He, Japin Li
Discussion: https://postgr.es/m/CAGECzQT3caUbcCcszNewCCmMbCuyP7XNAm60J3ybd6PN5kH2Dw@mail.gmail.com
A recent commit added a copy_function member to the
dshash_parameters struct, but it missed updating a couple of
comments that refer to the function pointer members of this struct.
One of those comments also refers to a tranche_name member and non-
arg variants of the function pointer members, all of which were
either removed during development or removed shortly after dshash
table support was committed.
Oversights in commits 8c0d7bafad, d7694fc148, and 42a1de3013.
Discussion: https://postgr.es/m/20240227045213.GA2329190%40nathanxps13
This is a followup to commit 66ea94e8e6.
Error mssages concerning incorrect formats for date-time types are
unified and parameterized, instead of using a fully separate error
message for each type.
Similarly, error messages regarding numeric and string arguments to
certain items are standardized, and instead of saying that the argument
is out of range simply say that it is invalid. The actual invalid
arguments to these itesm are now shown in the error message.
Error messages relating to numeric inputs of Nan or Infinity are
made more informative.
Jeevan Chalke and Kyotaro Horiguchi, with some input from Tom Lane.
Discussion: https://postgr.es/m/20240129.121200.235012930453045390.horikyota.ntt@gmail.com
object_classes[] provided unnecessary indirection between catalog OIDs
and the enum ObjectClass when calling add_object_address(). This array
has been originally introduced in 30ec31604d and was useful because not
all relation OIDs were compile-time constants back then, which has not
been the case for a long time now for all the elements of ObjectClass.
This commit removes object_classes[], switching to the catalog OIDs
when calling add_object_address(). This shaves some code while saving
in maintenance because it was necessary to maintain the enum ObjectClass
and the array in sync when adding new object types.
Reported-by: Jeff Davis
Author: Jelte Fennema-Nio
Reviewed-by: Jian He, Michael Paquier
Discussion: https://postgr.es/m/CAGECzQT3caUbcCcszNewCCmMbCuyP7XNAm60J3ybd6PN5kH2Dw@mail.gmail.com
Many modern compilers are able to optimize function calls to functions
where the parameters of the called function match a leading subset of
the calling function's parameters. If there are no instructions in the
calling function after the function is called, then the compiler is free
to avoid any stack frame setup and implement the function call as a
"jmp" rather than a "call". This is called sibling call optimization.
Here we adjust the memory allocation functions in mcxt.c to allow this
optimization. This requires moving some responsibility into the memory
context implementations themselves. It's now the responsibility of the
MemoryContext to check for malloc failures. This is good as it both
allows the sibling call optimization, but also because most small and
medium allocations won't call malloc and just allocate memory to an
existing block. That can't fail, so checking for NULLs in that case
isn't required.
Also, traditionally it's been the responsibility of palloc and the other
allocation functions in mcxt.c to check for invalid allocation size
requests. Here we also move the responsibility of checking that into the
MemoryContext. This isn't to allow the sibling call optimization, but
more because most of our allocators handle large allocations separately
and we can just add the size check when doing large allocations. We no
longer check this for non-large allocations at all.
To make checking the allocation request sizes and ERROR handling easier,
add some helper functions to mcxt.c for the allocators to use.
Author: Andres Freund
Reviewed-by: David Rowley
Discussion: https://postgr.es/m/20210719195950.gavgs6ujzmjfaiig@alap3.anarazel.de
Presently, string keys are not well-supported for dshash tables.
The dshash code always copies key_size bytes into new entries'
keys, and dshash.h only provides compare and hash functions that
forward to memcmp() and tag_hash(), both of which do not stop at
the first NUL. This means that callers must pad string keys so
that the data beyond the first NUL does not adversely affect the
results of copying, comparing, and hashing the keys.
To better support string keys in dshash tables, this commit does
a couple things:
* A new copy_function field is added to the dshash_parameters
struct. This function pointer specifies how the key should be
copied into new table entries. For example, we only want to copy
up to the first NUL byte for string keys. A dshash_memcpy()
helper function is provided and used for all existing in-tree
dshash tables without string keys.
* A set of helper functions for string keys are provided. These
helper functions forward to strcmp(), strcpy(), and
string_hash(), all of which ignore data beyond the first NUL.
This commit also adjusts the DSM registry's dshash table to use the
new helper functions for string keys.
Reviewed-by: Andy Fan
Discussion: https://postgr.es/m/20240119215941.GA1322079%40nathanxps13
A couple of dshash_create() callers provide 0 for the 'void *arg'
argument, which might give readers the incorrect impression that
this is some sort of "flags" parameter.
Reviewed-by: Andy Fan
Discussion: https://postgr.es/m/20240119215941.GA1322079%40nathanxps13
Add a note about the additional privileges required after the fix in
4989ce7264 (wording per Tom Lane); also change marked-up mentions of
"target_table_name" to be simply "the target table" or the like. Also,
note that "join_condition" is scouted for requisite privileges.
Backpatch to 15.
Discussion: https://postgr.es/m/202402211653.zuh6objy3z72@alvherre.pgsql
Information of sequences is cached for each backend for currval() and
nextval(), and the update of some cached information was mixed in the
middle of computations based on the other properties of a sequence, for
the increment value in nextval() and the cached state when altering a
sequence.
Grouping them makes the code easier to follow and to refactor in the
future, when splitting the computation and the SeqTable change parts.
Note that the cached data is untouched between the areas where these
cache updates are moved.
Issue noticed while doing some refactoring of the sequence code.
Author: Michael Paquier
Reviewed-by: Tomas Vondra
Discussion: https://postgr.es/m/ZWlohtKAs0uVVpZ3@paquier.xyz
Similarly to tables and indexes, these functions are able to open
relations with a sequence relkind, which is useful to make a distinction
with the other relation kinds. Previously, commands/sequence.c used a
mix of table_{close,open}() and relation_{close,open}() routines when
manipulating sequence relations, so this clarifies the code.
A direct effect of this change is to align the error messages produced
when attempting DDLs for sequences on relations with an unexpected
relkind, like a table or an index with ALTER SEQUENCE, providing an
extra error detail about the relkind of the relation used in the DDL
query.
Author: Michael Paquier
Reviewed-by: Tomas Vondra
Discussion: https://postgr.es/m/ZWlohtKAs0uVVpZ3@paquier.xyz
When this assertion was installed (in commit d2f60a3ab), I thought
it was only for catching server logic errors that caused accesses to
catalogs that were undergoing index rebuilds. However, it will also
fire in case of a user-defined index expression that attempts to
access its own table. We occasionally see reports of people trying
to do that, and typically getting unintelligible low-level errors
as a result. We can provide a more on-point message by making this
a regular runtime check.
While at it, adjust the similar error check in
systable_beginscan_ordered to use the same message text. That one
is (probably) not reachable without a coding bug, but we might as
well use a translatable message if we have one.
Per bug #18363 from Alexander Lakhin. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/18363-e3598a5a572d0699@postgresql.org
51efe38cb9 introduced bunch of tests for idle_in_transaction_session_timeout,
transaction_timeout and statement_timeout. These tests were too flaky on some
slow buildfarm machines, so we plan to replace them with TAP tests using
injection points. This commit removes flaky tests.
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
Author: Andrey Borodin
This commit introduces a new field 'sublevels_up' in ReplaceVarnoContext,
and enhances replace_varno_walker() to:
1) recurse into subselects with sublevels_up increased, and
2) perform the replacement only when varlevelsup is equal to sublevels_up.
This commit also fixes some outdated comments. And besides adding relevant
test cases, it makes some unification over existing SJE test cases.
Discussion: https://postgr.es/m/CAMbWs4-%3DPO6Mm9gNnySbx0VHyXjgnnYYwbN9dth%3DTLQweZ-M%2Bg%40mail.gmail.com
Author: Richard Guo
Reviewed-by: Andrei Lepikhov, Alexander Korotkov
build_child_join_sjinfo creates a derived SpecialJoinInfo in
the short-lived GEQO context, but afterwards the semi_rhs_exprs
from that may be used in a UniquePath for a child base relation.
This breaks the expectation that all base-relation-level structures
are in the planning-lifespan context, leading to use of a dangling
pointer with probable ensuing crash later on in create_unique_plan.
To fix, copy the expression trees when making a UniquePath.
Per bug #18360 from Alexander Lakhin. This has been broken since
partitionwise joins were added, so back-patch to all supported
branches.
Discussion: https://postgr.es/m/18360-a23caf3157f34e62@postgresql.org
The new facility makes it easier to optimize bulk loading, as the
logic for buffering, WAL-logging, and syncing the relation only needs
to be implemented once. It's also less error-prone: We have had a
number of bugs in how a relation is fsync'd - or not - at the end of a
bulk loading operation. By centralizing that logic to one place, we
only need to write it correctly once.
The new facility is faster for small relations: Instead of of calling
smgrimmedsync(), we register the fsync to happen at next checkpoint,
which avoids the fsync latency. That can make a big difference if you
are e.g. restoring a schema-only dump with lots of relations.
It is also slightly more efficient with large relations, as the WAL
logging is performed multiple pages at a time. That avoids some WAL
header overhead. The sorted GiST index build did that already, this
moves the buffering to the new facility.
The changes to pageinspect GiST test needs an explanation: Before this
patch, the sorted GiST index build set the LSN on every page to the
special GistBuildLSN value, not the LSN of the WAL record, even though
they were WAL-logged. There was no particular need for it, it just
happened naturally when we wrote out the pages before WAL-logging
them. Now we WAL-log the pages first, like in B-tree build, so the
pages are stamped with the record's real LSN. When the build is not
WAL-logged, we still use GistBuildLSN. To make the test output
predictable, use an unlogged index.
Reviewed-by: Andres Freund
Discussion: https://www.postgresql.org/message-id/30e8f366-58b3-b239-c521-422122dd5150%40iki.fi
943f7ae1c8 has changed GetSlotInvalidationCause() so as it would
return the last element of SlotInvalidationCauses[] when an incorrect
conflict reason name is given by a caller, but this should return
RS_INVAL_NONE in such cases, even if such a state should never be
reached in practice.
Per gripe from Peter Smith.
Reviewed-by: Bharath Rupireddy
Discussion: https://postgr.es/m/CAHut+PtsrSWxczpGkSaSVtJo+BXrvJ3Hwp5gES14bbL-G+HL7A@mail.gmail.com
By enabling slot synchronization, all the failover logical replication
slots on the primary (assuming configurations are appropriate) are
automatically created on the physical standbys and are synced
periodically. The slot sync worker on the standby server pings the primary
server at regular intervals to get the necessary failover logical slots
information and create/update the slots locally. The slots that no longer
require synchronization are automatically dropped by the worker.
The nap time of the worker is tuned according to the activity on the
primary. The slot sync worker waits for some time before the next
synchronization, with the duration varying based on whether any slots were
updated during the last cycle.
A new parameter sync_replication_slots enables or disables this new
process.
On promotion, the slot sync worker is shut down by the startup process to
drop any temporary slots acquired by the slot sync worker and to prevent
the worker from trying to fetch the failover slots.
A functionality to allow logical walsenders to wait for the physical will
be done in a subsequent commit.
Author: Shveta Malik, Hou Zhijie based on design inputs by Masahiko Sawada and Amit Kapila
Reviewed-by: Masahiko Sawada, Bertrand Drouvot, Peter Smith, Dilip Kumar, Ajin Cherian, Nisha Moond, Kuroda Hayato, Amit Kapila
Discussion: https://postgr.es/m/514f6f2f-6833-4539-39f1-96cd1e011f23@enterprisedb.com
The reason was that the ALTER SUBSCRIPTION .. SET PUBLICATION will lead to
the restarting of apply worker and after the restart, the apply worker
will use the existing slot and replication origin corresponding to the
subscription. Now, it is possible that before restart the origin has not
been updated and the WAL start location points to a location before where
PUBLICATION exists which can lead to the error "publication ... does not
exist".
Fix it by recreating the subscription as a newly created subscription will
start processing WAL from the recent WAL location and will see the
required publication.
This behavior has existed from the time logical replication was introduced
but is exposed by this test and we have started a discussion for a better
fix for this problem.
As per Buildfarm
Diagnosed-by: Amit Kapila
Author: Vignesh C
Discussion: https://postgr.es/m/3307255.1706911634@sss.pgh.pa.us
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
Allegedly, only certain fields of the Constraint node are valid based
on contype. But this has historically not been kept up to date in the
read/write functions. The Constraint node is only used for debugging
DDL statements, so there are no strong requirements for its output,
and there is no enforcement for its correctness. (There was no read
support before a6bc3301925.) Commits e7a552f303 and abf46ad9c7 are
examples of where omissions were fixed.
This patch just removes the custom read/write implementations for the
Constraint node type. Now we just output all the fields, which is a
bit more than before, but at least we don't have to maintain these
functions anymore. Also, we lose the string representation of the
contype field, but for this marginal use case that seems tolerable.
This patch also changes the documentation of the Constraint struct to
put less emphasis on grouping fields by constraint type but rather
document for each field how it's used.
Reviewed-by: Paul Jungwirth <pj@illuminatedcomputing.com>
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
Since the size of the string representation of an uuid is fixed, there
is no benefit in using a StringInfo. This commit simplifies uuid_oud()
to not rely on a StringInfo, where avoiding the overhead of the string
manipulation makes the function substantially faster.
A COPY TO on a relation with one UUID attribute can show up to a 40%
speedup when the bottleneck is the COPY computation with uuid_out()
showing up at the top of the profiles (numbered measure here, Laurenz
has mentioned something closer to 20% faster runtimes), for example when
the data is fully in shared buffers or the OS cache.
Author: Laurenz Albe
Reviewed-by: Andres Freund, Michael Paquier
Description: https://postgr.es/m/679d5455cbbb0af667ccb753da51a475bae1eaed.camel@cybertec.at
This commit switches the handling of the conflict cause strings for
replication slots to use a table rather than being explicitly listed,
using a C99-designated initializer syntax for the array elements. This
makes the whole more readable while easing future maintenance with less
areas to update when adding a new conflict reason.
This is similar to 74a7306310, but the scale of the change is smaller
as there are less conflict causes than LWLock builtin tranche names.
Author: Bharath Rupireddy
Reviewed-by: Jelte Fennema-Nio
Discussion: https://postgr.es/m/CALj2ACUxSLA91QGFrJsWNKs58KXb1C03mbuwKmzqqmoAKLwJaw@mail.gmail.com
Verify that a user running MERGE with a DO NOTHING clause has
privileges to read the table, even if no columns are referenced. Such
privileges were already required if the ON clause or any of the WHEN
conditions referenced any column at all, so there's no functional change
in practice.
This change fixes an assertion failure in the case where no column is
referenced by the command and the WHEN clauses are all DO NOTHING.
Backpatch to 15, where MERGE was introduced.
Reported-by: Alena Rybakina <a.rybakina@postgrespro.ru>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/4d65a385-7efa-4436-a825-0869f89d9d92@postgrespro.ru
This option is useful to bypass the default behavior of init() which
would create the data folder of a new cluster by copying it from a
template previously initdb'd, if any. Copying the data folder is much
cheaper than running initdb, but some tests may want to force that. For
example, one scenario of pg_combinebackup updated in this commit needs a
different system ID for two nodes.
Previously, this could only be achieved by unsetting
$ENV{'INITDB_TEMPLATE'}, which could become a problem in complex node
setups by making tests less efficient.
Author: Amul Sul
Reviewed-by: Robert Haas, Michael Paquier
Discussion: https://postgr.es/m/Zc1tX9lLonLGu6oH@paquier.xyz
The explanation of interval's behavior in datatype.sgml wasn't wrong
exactly, but it was unclear, partly because it buried the lede about
there being three internal fields. Rearrange and wordsmith for more
clarity.
The discussion of extract() claimed that input of type date was
handled by casting, but actually there's been a separate SQL function
taking date for a very long time. Also, it was mostly silent about
how interval inputs are handled, but there are several field types
for which it seems useful to be specific.
Improve discussion of justify_days()/justify_hours() too.
In passing, remove vertical space in some groups of examples,
as there was little consistency about whether to have such space
or not. (I only did this within the datetime functions section;
there are some related inconsistencies elsewhere.)
Per discussion of bug #18348 from Michael Bondarenko. There
may be some code changes coming out of that discussion too,
but we likely won't back-patch them. This docs-only patch
seems useful to back-patch, though I only carried it back to
v13 because it didn't apply easily in v12.
Discussion: https://postgr.es/m/18348-b097a3587dfde8a4@postgresql.org
When the partition pruning code finds an OpExpr with an operator that
does not belong to the partition key's opfamily, the code checks to see
if the negator of the operator is the opfamily's BTEqualStrategyNumber
operator so that partition pruning can support that operator and invert
the matching partitions. Doing this only works for LIST partitioned
tables.
Here we fix a minor correctness issue where when we discover we're not
pruning for a LIST partitioned table, we return PARTCLAUSE_NOMATCH.
PARTCLAUSE_NOMATCH is only meant to be used when the clause may match
another partitioned key column. For this case, the clause is not going
to be any more useful to another partitioned key as the partition strategy
is not going to change from one key to the next.
Noticed while working 4c2369ac5. No backpatch because returning
PARTCLAUSE_NOMATCH instead of PARTCLAUSE_UNSUPPORTED mostly just causes
wasted effort checking subsequent partition keys against a clause that
will never be used for pruning.
In passing, correct a comment for get_matching_range_bounds() which
mentions that an 'opstrategy' of 0 is supported. It's not, so fix the
comment. This was pointed out by Alexander Lakhin.
Discussion: https://postgr.es/m/CAApHDvqriy8mPOFJ_Bd66YGXJ4+XULpv-4YdB+ePdCQFztyisA@mail.gmail.com
Discussion: https://postgr.es/m/312fb507-9b5e-cf83-d8ed-cd0da72a902c@gmail.com
The invalidation of an active slot is done in two steps:
- Termination of the backend holding it, if any.
- Report that the slot is obsolete, with a conflict cause depending on
the slot's data.
This can be racy because between these two steps the slot mutex would be
released while doing system calls, which means that the effective_xmin
and effective_catalog_xmin could advance during that time, detecting a
conflict cause different than the one originally wanted before the
process owning a slot is terminated.
Holding the mutex longer is not an option, so this commit changes the
code to record the LSNs stored in the slot during the termination of the
process owning the slot.
Bonus thanks to Alexander Lakhin for the various tests and the analysis.
Author: Bertrand Drouvot
Reviewed-by: Michael Paquier, Bharath Rupireddy
Discussion: https://postgr.es/m/ZaTjW2Xh+TQUCOH0@ip-10-97-1-34.eu-west-3.compute.internal
Backpatch-through: 16
Partition pruning wrongly assumed that, for a table partitioned on a
boolean column, a clause in the form "boolcol IS NOT false" and "boolcol
IS NOT true" could be inverted to correspondingly become "boolcol IS true"
and "boolcol IS false". These are not equivalent as the NOT version
matches the opposite boolean value *and* NULLs. This incorrect assumption
meant that partition pruning pruned away partitions that could contain
NULL values.
Here we fix this by correctly not pruning partitions which could store
NULLs.
To be affected by this, the table must be partitioned by a NULLable boolean
column and queries would have to contain "boolcol IS NOT false" or "boolcol
IS NOT true". This could result in queries filtering out NULL values
with a LIST partitioned table and "ERROR: invalid strategy number 0"
for RANGE and HASH partitioned tables.
Reported-by: Alexander Lakhin
Bug: #18344
Discussion: https://postgr.es/m/18344-8d3f00bada6d09c6@postgresql.org
Backpatch-through: 12
Before the previous commit, the test could hang until
LOG_SNAPSHOT_INTERVAL_MS (15s), until checkpoint_timeout (300s), or
indefinitely. An indefinite hang was awfully improbable. It entailed
the test reaching checkpoint_timeout before the
DecodingContextFindStartpoint() of a CREATE SUBSCRIPTION, yet after the
preceding WAL record. Back-patch to v16, which introduced the test.
Bertrand Drouvot, reported by Noah Misch.
Discussion: https://postgr.es/m/20240211010227.a2.nmisch@google.com
One IPC::Run::start() used an IPC::Run::timer() without checking for
expiration. The other used no timeout or timer. Back-patch to v16,
which introduced the test.
Reviewed by Bertrand Drouvot.
Discussion: https://postgr.es/m/20240211010227.a2.nmisch@google.com
Michael Paquier
Nathan Bossart
Andrew Dunstan
David Rowley
Alvaro Herrera
Peter Eisentraut
Tom Lane
Alexander Korotkov
Heikki Linnakangas
Amit Kapila
Daniel Gustafsson
Noah Misch