Certain cases involving the use of cursors had assertion failures within
_bt_preprocess_keys's recently added no-op return path. The assertion
in question made the faulty assumption that a second or third call to
_bt_preprocess_keys (within the same btrescan) could only happen when
another scheduled primitive index scan was just about to begin.
It would be possible to address the problem by only allowing scans that
have array keys to take the new no-op path, forcing affected cases to
perform redundant preprocessing work. It seems simpler to just remove
the assertion, and reframe the no-op path as a more general mechanism.
Take this simpler approach.
The important underlying principle is that we only need to perform
preprocessing once per btrescan (at most). This is expected regardless
of whether or not the scan happens to have array keys.
Oversight in commit 1b134ca5, which enhanced nbtree ScalarArrayOp
execution.
Reported-By: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/ef0f7c8b-a6fa-362e-6fd6-054950f947ca@gmail.com
pg_dump/restore failed to properly set the table access method for
partitioned tables, as it relies on SET queries that would change
default_table_access_method. However, SET affects only tables and
materialized views, not partitioned tables which would always be
restored with their pg_class.relam set to 0, losing their table AM set
by either a CREATE TABLE .. USING or by a ALTER TABLE .. SET ACCESS
METHOD.
Appending a USING clause to the definition of CREATE TABLE is not
possible as users may specify --no-table-access-method at restore or for
a dump, meaning that the table AM portions may have to be skipped.
Rather than SET, the solution used by this commit is to generate an
extra ALTER TABLE .. SET ACCESS METHOD when restoring a partitioned
table, based on the table AM set in its TOC entry. The choice of using
a SET query or an ALTER TABLE query for a relation requires the addition
of the relkind to the TOC entry to be able to choose between one or the
other. Note that using ALTER TABLE SET ACCESS METHOD on a relation with
physical storage would require a full rewrite, which would be costly for
one. This also creates problems with binary upgrades where the rewrite
would not be able to keep the OID of the relation consistent across the
upgrade.
This commit would normally require a protocol bump, but a45c78e328 has
already done one for this release cycle.
Regression tests are adjusted with the new expected output, with some
tweaks for the table AMs of the partitions to make the output more
readable.
Issue introduced by 374c7a2290, that has added support for table AMs
in partitioned tables.
Author: Michael Paquier
Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/Zh4JLSvvtQgBJZkZ@paquier.xyz
When specifying the createdb strategy, the documentation suggests valid
options are FILE_COPY and WAL_LOG, but the code does case-sensitive
comparison and accepts only "file_copy" and "wal_log" as valid.
Fixed by doing a case-insensitive comparison using pg_strcasecmp(), same
as for other string parameters nearby.
While at it, apply fmtId() to a nearby "locale_provider". This already
did the comparison in case-insensitive way, but the value would not be
double-quoted, confusing the parser and the error message.
Backpatch to 15, where the strategy was introduced.
Backpatch-through: 15
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/90c6913a-1dd2-42b4-8365-ce3b09c39b17@enterprisedb.com
This should have the same results for all practical purposes.
The advantage of selecting 'GMT' is that it's guaranteed to work
even when the remote system's timezone database is missing
entries, because pg_tzset() hard-wires handling of that,
at least in 9.2 and later.
(It seems like it would be a good idea to similarly hard-wire
correct handling of 'UTC', but that'll be a little more invasive
than I want to consider back-patching. Leave that for another
day when we're not in feature freeze.)
Per trouble report from Adnan Dautovic. Back-patch to all
supported branches.
Discussion: https://postgr.es/m/465248.1712211585@sss.pgh.pa.us
The documents were clear that queryid should not be assumed to be stable
between major versions but said nothing about minor versions and left
the reader to guess if that was implied by the mention of the
instability of queryid between major versions.
Here we give minor versions an explicit mention to indicate queryid can
generally be assumed stable between minor versions.
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/CAApHDvpYGE6h0cD9UO-eHySPynPj1L3J%3DHxT%2BA7Ud8_Yo6AuzA%40mail.gmail.com
Backpatch-through: 12
Commit 6bf5c42b55 cannot work on Windows,
because it lacks symlink support. While the bug fix in commit
cd64dc42d1 is correct as far as I know,
the test case changes depend on the previous commit, so this will
have to live without test coverage until we can come up with a better
solution. Commit fa7036dd66 was a test
case bug fix on top of those two, to prevent failures on Linux, so that
has to come out as well.
Per the buildfarm, CI, and Thomas Munro.
After cd64dc42d1, a significant
percentage of the buildfarm got unhappy, because pg_basebackup chokes
if it tries to create a tarfile with symlink more than 99 characters
in length. To try to fix that problem, use tempdir_short instead of
tempdir, as we do in pg_verifybackup's 003_corruption.pl.
There's a more complicated workaround for the same issue in
pg_basebackup's 010_pg_basebackup.pl, but I'm not clear whether
there's any reason to do it that way here. For now, let's try this,
to at least get the buildfarm green again.
A better long-term fix would be to figure out how to generate tar
files containing long symlinks, but that will have to wait for
another time.
The previous coding mangled the pathname calculation for
incremental files located in user-defined tablespaces.
Enhance the test cases to cover such cases, as I should have
done originally. Thanks to Andres Freund for alerting me to the
lack of test coverage.
Discussion: http://postgr.es/m/CA+TgmoYdXTjo9iQeoipTccDpWZzvBNS6EndY2uARM+T4yG_yDg@mail.gmail.com
This commit doesn't use this infrastructure for anything new, although
it does adapt 010_pg_basebackup.pl to use it. However, a future commit
will use this to improve test coverage for pg_combinebackup.
Patch by me, reviewed (but not fully endorsed) by Andres Freund.
Discussion: http://postgr.es/m/CA+TgmoYdXTjo9iQeoipTccDpWZzvBNS6EndY2uARM+T4yG_yDg@mail.gmail.com
The optimization for inserts into BRIN indexes added by c1ec02be1d
relies on a cache that needs to be explicitly released after calling
index_insert(). The commit however failed to invoke the cleanup in
validate_index(), which calls index_insert() indirectly through
table_index_validate_scan().
After inspecting index_insert() callers, it seems unique_key_recheck()
is missing the call too.
Fixed by adding the two missing index_insert_cleanup() calls.
The commit does two additional improvements. The aminsertcleanup()
signature is modified to have the index as the first argument, to make
it more like the other AM callbacks. And the aminsertcleanup() callback
is invoked even if the ii_AmCache is NULL, so that it can decide if the
cleanup is necessary.
Author: Alvaro Herrera, Tomas Vondra
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/202401091043.e3nrqiad6gb7@alvherre.pgsql
These operators were removed by 2f70fdb064 in the v14 cycle but they were
accidentally left in the table of build-in operator classes. Backpatch down
to v14 where the operators where removed.
Author: Aleksander Alekseev <aleksander@timescale.com>
Reported-by: Colin Caine <cmcaine@gmail.com>
Discussion: https://postgr.es/m/CADwQTQbbr2UQ_fpbyc+8ay=RwEYgYk=TZxH3+RHDqAQfoG+EWA@mail.gmail.com
Backpatch-through: v14
It is possible for certain cases to remove not-null constraints without
maintaining the attnotnull in its correct state; for example if you drop
a column that's part of the primary key, and the other columns of the PK don't
have not-null constraints, then we should reset the attnotnull flags for
those other columns; up to this commit, we didn't. Handle those cases
better by doing the attnotnull reset in RemoveConstraintById() instead
of in dropconstraint_internal().
However, there are some cases where we must not do so. For example if
those other columns are in replica identity indexes or are generated
identity columns, we must keep attnotnull set, even though it results in
the catalog inconsistency that no not-null constraint supports that.
Because the attnotnull reset now happens in more places than before, for
instance when a column of the primary key changes type, we need an
additional trick to reinstate it as necessary. Introduce a new
alter-table pass that does this, which needs simply reschedule some
AT_SetAttNotNull subcommands that were already being generated and
ignored.
Because of the exceptions in which attnotnull is not reset noted above,
we also include a pg_dump hack to include a not-null constraint when the
attnotnull flag is set even if no pg_constraint row exists. This part
is undesirable but necessary, because failing to handle the case can
result in unrestorable dumps.
Reported-by: Tender Wang <tndrwang@gmail.com>
Co-authored-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: jian he <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CAHewXN=hMbNa3d43NOR=OCgdgpTt18S-1fmueCoEGesyeK4bqw@mail.gmail.com
The function declaration for select_next_encryption_method use the
variable name have_valid_connection, so fix the prototype in the
header to match that.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/3F577953-A29E-4722-98AD-2DA9EFF2CBB8@yesql.se
This updates the link from pg_createsubscriber to initial data sync
to actually link to the subsection in question as opposed to the
main logical replication section.
Author: Pavel Luzanov <p.luzanov@postgrespro.ru>
Discussion: https://postgr.es/m/a4af555a-ac60-4416-877d-0440d29b8763@postgrespro.ru
This fixes various typos, duplicated words, and tiny bits of whitespace
mainly in code comments but also in docs.
Author: Daniel Gustafsson <daniel@yesql.se>
Author: Heikki Linnakangas <hlinnaka@iki.fi>
Author: Alexander Lakhin <exclusion@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Author: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://postgr.es/m/3F577953-A29E-4722-98AD-2DA9EFF2CBB8@yesql.se
Preprocessing for nbtree index scans allowed array "input" scan keys
already marked eliminated during array-specific preprocessing to be
"fixed up" during preprocessing proper. This allowed eliminated scan
keys on DESC index columns to spurious have their strategy commuted,
causing assertion failures.
To fix, teach _bt_fix_scankey_strategy to ignore these scan keys. This
brings it in line with its only caller, _bt_preprocess_keys.
Oversight in commit 5bf748b8, which enhanced nbtree ScalarArrayOp
execution.
Reported-By: Donghang Lin <donghanglin@gmail.com>
Discussion: https://postgr.es/m/CAA=D8a2sHK6CAzZ=0CeafC-Y-MFXbYxnRSHvZTi=+JHu6kAa8Q@mail.gmail.com
Previously, they were recognized anywhere in an incremental backup
directory; now, we restrict this to places where they are expected to
appear. That means this code will need updating if we ever do
incremental backups of files in other places (e.g. SLRU files), but
it lets you create a file called INCREMENTAL.config (or something like
that) at the top level of the data directory and still have things
work.
Patch by me, per request from David Steele, who also reviewed.
Discussion: http://postgr.es/m/5a7817da-6349-4653-8056-470300b6e512@pgmasters.net
This part of my previous commit seems to have broken pg_upgrade on
crake, at least from 9.2. I'll see if there's a better fix, but in the
meantime this should suffice to keep the buildfarm green.
We don't want users to think that pg_combinebackup is trying to check
the validity of individual backups, because it isn't. Adjust the wording
about sanity checks to make it clear that verification of individual
backups is the job of pg_verifybackup, and that the checks performed
by pg_combinebackup are around the relationships between the backups.
Per discussion with David Steele.
Discussion: http://postgr.es/m/e6f930c3-590c-47b9-b094-217bb2a3e22e@pgmasters.net
In tables with primary keys, pg_dump creates tables with primary keys by
initially dumping them with throw-away not-null constraints (marked "no
inherit" so that they don't create problems elsewhere), to later drop
them once the primary key is restored. Because of a unrelated
consideration, on tables with children we add not-null constraints to
all columns of the primary key when it is created.
If both a table and its child have primary keys, and pg_dump happens to
emit the child table first (and its throw-away not-null) and later its
parent table, the creation of the parent's PK will fail because the
throw-away not-null constraint collides with the permanent not-null
constraint that the PK wants to add, so the dump fails to restore.
We can work around this problem by letting the primary key "take over"
the child's not-null. This requires no changes to pg_dump, just two
changes to ALTER TABLE: first, the ability to convert a no-inherit
not-null constraint into a regular inheritable one (including recursing
down to children, if there are any); second, the ability to "drop" a
constraint that is defined both directly in the table and inherited from
a parent (which simply means to mark it as no longer having a local
definition).
Secondarily, change ATPrepAddPrimaryKey() to acquire locks all the way
down the inheritance hierarchy, in case we need to recurse when
propagating constraints.
These two changes allow pg_dump to reproduce more cases involving
inheritance from versions 16 and older.
Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
constraint that's marked as inherited; this allows a dump to restore
with no errors if a table with a PK inherits from another which also has
a PK; 2) avoid giving inherited constraints throwaway names, for the
rare cases where such a constraint survives after the restore.
Reported-by: Andrew Bille <andrewbille@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA@mail.gmail.com
Discussion: https://postgr.es/m/Zh0aAH7tbZb-9HbC@pryzbyj2023
c0fc075186 wasn't careful about naming the DOMAIN used in some new
tests in sqljson_queryfunc.sql so as not to clash with the name of a
DOMAIN used in the nearby sqljson_jsontable.sql. Fix by using a
different name for the newly added DOMAIN in sqljson_queryfuncs.sql.
Per buildfarm members canebrake and urutu.
Discussion: https://postgr.es/m/CA+HiwqEjkbDxqqD3VJamc6R9+B102H7=SFYYOM7gKrxzJO35TQ@mail.gmail.com
This addresses some post-commit review comments for commits 6185c973,
de3600452, and 9425c596a0, with the following changes:
* Fix JSON_TABLE() syntax documentation to use the term
"path_expression" for JSON path expressions instead of
"json_path_specification" to be consistent with the other SQL/JSON
functions.
* Fix a typo in the example code in JSON_TABLE() documentation.
* Rewrite some newly added comments in jsonpath.h.
* In JsonPathQuery(), add missing cast to int before printing an enum
value.
Reported-by: Jian He <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CACJufxG_e0QLCgaELrr2ZNz7AxPeGCNKAORe3fHtFCQLsH4J4Q@mail.gmail.com
SQL/JSON query functions allow specifying an expression to return
when either of ON ERROR or ON EMPTY condition occurs when evaluating
the JSON path expression. The parser (transformJsonBehavior()) checks
that the specified expression is one of the supported expressions, but
there are two issues with how the check is done that are fixed in this
commit:
* No check for some expressions related to coercion, such as
CoerceViaIO, that may appear in the transformed user-specified
expressions that include cast(s)
* An unsupported expression may be masked by a coercion-related
expression, which must be flagged by checking the latter's
argument expression recursively
Author: Jian He <jian.universality@gmail.com>
Author: Amit Langote <amitlangote09@gmail.com>
Reported-by: Jian He <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CACJufxEqhqsfrg_p7EMyo5zak3d767iFDL8vz_4%3DZBHpOtrghw@mail.gmail.com
Discussion: https://postgr.es/m/CACJufxGOerH1QJknm1noh-Kz5FqU4p7QfeZSeVT2tN_4SLXYNg@mail.gmail.com
This improves some error messages emitted by SQL/JSON query functions
by mentioning column name when available, such as when they are
invoked as part of evaluating JSON_TABLE() columns. To do so, a new
field column_name is added to both JsonFuncExpr and JsonExpr that is
only populated when creating those nodes for transformed JSON_TABLE()
columns.
While at it, relevant error messages are reworded for clarity.
Reported-by: Jian He <jian.universality@gmail.com>
Suggested-by: Jian He <jian.universality@gmail.com>
Discussion: https://postgr.es/m/CACJufxG_e0QLCgaELrr2ZNz7AxPeGCNKAORe3fHtFCQLsH4J4Q@mail.gmail.com
fefd9a3fed turned tail recursion of CommitTransactionCommand() and
AbortCurrentTransaction() into iteration. However, it splits the handling of
cases between different functions.
This commit puts the handling of all the cases into
AbortCurrentTransactionInternal() and CommitTransactionCommandInternal().
Now CommitTransactionCommand() and AbortCurrentTransaction() are just doing
the repeated calls of internal functions.
Reported-by: Andres Freund
Discussion: https://postgr.es/m/20240415224834.w6piwtefskoh32mv%40awork3.anarazel.de
Author: Andres Freund
GlobalVisTestNonRemovableHorizon() was only used for the implementation of
snapshot_too_old, which was removed in f691f5b80a. As using
GlobalVisTestNonRemovableHorizon() is not particularly efficient, no new uses
for it should be added. Therefore remove.
Discussion: https://postgr.es/m/20240415185720.q4dg4dlcyvvrabz4@awork3.anarazel.de
Commit b437571714 added support for parallel builds of BRIN indexes,
using code similar to BTREE. But there were to be a couple unnecessary
differences, particularly in how the leader waits for the workers, and
merges the results. So remove these, to make the code more similar.
The leader never waited on the workersdonecv condition variable, but
simply called WaitForParallelWorkersToFinish() in _brin_end_parallel()
and then merged the per-worker results. This worked correctly, but it
seems better to do the wait and merge before _brin_end_parallel().
This commit moves the relevant code to _brin_parallel_heapscan/merge(),
which means _brin_end_parallel() remains responsible only for exiting
the parallel mode and accumulating WAL usage data.
Discussion: https://postgr.es/m/3733d042-71e1-6ae6-5fac-00c12db62db6@enterprisedb.com
As explained in 4d916dd876, the test instability is caused by delayed
cleanup of deleted rows. This commit removes the DELETE, stabilizing the
test without accidentally disabling parallel builds.
The intent of the delete however was to produce empty ranges, and test
that the parallel index build populates those correctly. But there's
another way to create empty ranges - partial indexes, which does not
rely on cleanup of deleted rows.
Idea to use partial indexes by Matthias van de Meent, patch by me.
Discussion: https://postgr.es/m/95d9cd43-5a92-407c-b7e4-54cd303630fe%40enterprisedb.com
The configure check for HAVE_DECL_LLVMORCREGISTERPERF was removed by
e9a9843e13, but some code guarded by it was left. (That commit
removed the "register" calls but left the "unregister" calls.) That
code cannot be reached anymore, so remove it.
Reported-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/5539b16c-cff7-46d5-9621-c3fb6b549e9e@iki.fi
The ON_ERROR option of the COPY command previously allowed omitting
its value, which was inconsistent with the syntax synopsis in the
documentation and the behavior of other non-boolean COPY options.
This change enforces providing a value for the ON_ERROR option,
ensuring consistency across other non-boolean options and aligning
with the documented syntax.
Author: Atsushi Torikoshi
Reviewed-by: Masahiko Sawada
Discussion: https://postgr.es/m/a9770bf57646d90dedc3d54cf32634b2%40oss.nttdata.com
BumpContext relies on using the head block from its 'blocks' field to
use as the current block to allocate new chunks to. When we receive an
allocation request larger than allocChunkLimit, we place these chunks on
a new dedicated block and, until now, we pushed the block onto the
*head* of the 'blocks' list.
This behavior caused the previous bump block to no longer be available
for new normal-sized (non-large) allocations and would result in blocks
only being partially filled if a large allocation request arrived before
the block became full.
Here adjust the code to push these dedicated blocks onto the *tail* of
the blocks list so that the head block remains intact and available to
be used by normal allocation request sizes until it becomes full.
In passing, make the elog(ERROR) calls for the unsupported callbacks
consistent. Likewise for the header comments for those functions.
Discussion: https://postgr.es/m/CAApHDvp9___r-ayJj0nZ6GD3MeCGwGZ0_6ZptWpwj+zqHtmwCw@mail.gmail.com
Discussion: https://postgr.es/m/CAApHDvqerXpzUnuDQfUEi3DZA+9=Ud9WSt3ruxN5b6PcOosx2g@mail.gmail.com
The code associated with EXEC SQL DEFINE was unreadable and full of
bugs, notably:
* It'd attempt to free a non-malloced string if the ecpg program
tries to redefine a macro that was defined on the command line.
* Possible memory stomp if user writes "-D=foo".
* Undef'ing or redefining a macro defined on the command line would
change the state visible to the next file, when multiple files are
specified on the command line. (While possibly that could have been
an intentional choice, the code clearly intends to revert to the
original macro state; it's just failing to consider this interaction.)
* Missing "break" in defining a new macro meant that redefinition
of an existing name would cause an extra entry to be added to the
definition list. While not immediately harmful, a subsequent undef
would result in the prior entry becoming visible again.
* The interactions with input buffering are subtle and were entirely
undocumented.
It's not that surprising that we hadn't noticed these bugs,
because there was no test coverage at all of either the -D
command line switch or multiple input files. This patch adds
such coverage (in a rather hacky way I guess).
In addition to the code bugs, the user documentation was confused
about whether the -D switch defines a C macro or an ecpg one, and
it failed to mention that you can write "-Dsymbol=value".
These problems are old, so back-patch to all supported branches.
Discussion: https://postgr.es/m/998011.1713217712@sss.pgh.pa.us
Peter Geoghegan