The interface list duplicates declarations in the headers and has
accumulated stale signatures and missing entries. Remove it rather
than maintaining a separate copy of the declarations.
Also correct the headings above AtPrepare_PredicateLocks() and
PostPrepare_PredicateLocks(), which incorrectly name the separate
AtPrepare_Locks() and PostPrepare_Locks() functions.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Aleksander Alekseev <aleksander@tigerdata.com>
Discussion: https://postgr.es/m/CAHGQGwHww5o8abim0DGVFNb5bBre2a+V1twhH2cu-J21rYGcaQ@mail.gmail.com
RelationReloadIndexInfo() re-reads the mutable pg_index fields
(indisready, indisvalid, indislive, etc.) only for indexes that are
not system relations. The TOAST index of a user table lives in
pg_toast and counts as one, but REINDEX CONCURRENTLY changes these
fields on it like on any other index.
A backend that had the TOAST indexes open when the invalidation
arrived kept indisready = false for the new index, and did not insert
its chunks there. Once the old index is dropped, reading such a value
fails with "missing chunk number 0 for toast value N in pg_toast_X".
The chunks remain in the TOAST table, only the index references were
gone, so a rebuild of the TOAST index would fix this issue.
This issue is fixed by re-reading the pg_index row for TOAST indexes,
filling in their relcache fields. This is the same as for an index on a
user table. Previouly, IsSystemRelation() was used, meaning the
pg_index data was not re-read for a TOAST relation or a relation with a
pinned OID (created by initdb). This check is meant for indexes whose
values never change, so IsCatalogRelation() is fine, implying that only
indexes created by initdb should be skipped (REINDEX CONCURRENTLY does
not work on catalogs).
This code is as old as 8ec943856a, and was only used for CREATE INDEX
CONCURRENTLY. 5dc92b844e, that has introduced REINDEX CONCURRENTLY,
has made possible to reach the problem with TOAST indexes due to these
getting concurrently built, validated and then swapped.
A test is added, able to reproduce the issue. It forces an insert
before a swap, with a second session that maintains the TOAST indexes
open while the new index becomes ready. This has been slightly
simplified by me compared to what the author has sent.
Oversight in 5dc92b844e.
Reported-by: Saicharanreddy Chinthareddy <saicharh@amazon.com>
Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CALj2ACUAv=7wjOV3pRFhQP4WHME_JazuhoCr01i4_F2XA=JKog@mail.gmail.com
Backpatch-through: 14
Here we add overflow checks to guard against overflowing the int8 sum of
all inputs for sum(int2), sum(int4), avg(int2) and avg(int4). Such an
overflow is very unlikely for the int2 versions, but somewhat possible
to hit for the int4 versions when given billions of large enough inputs.
Since our code and machines are getting faster, and with the advent of
things like columnar table AMs, it's getting more likely that such an
overflow could occur in a real use case.
No guards have been added to the average count. That would require 2^63
input rows, which is not possible to achieve in a single instance of
PostgreSQL, given other limitations such as 64-bit LSNs.
Fixing this only in master as we can probably get away with that today
due to this still being unlikely for users to ever hit.
Author: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/1ddc2a6f-4b26-43d9-9f3b-5b5db98a486d@gmail.com
Base backups read the control file (pg_control) from disk without any
protection against concurrent writes by the server, making it exposed to
torn copies. Attempting to use such a control file at startup would
cause a failure with a CRC not matching.
Base backups now read the control file while holding ControlFileLock.
c558e6fd92 has done the same for some SQL functions. The contents of
the file are saved in memory and sent over the wire as-is.
Backpatch down to v15. This issue is older than v15 but v14 is left out
of the picture as it is going to be EOL soon, so it does not seem worth
taking a risk for.
Author: David Steele <david@pgbackrest.org>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/d1697de1-eb7f-41bf-a4b7-bba7203df11f@pgbackrest.org
Backpatch-through: 15
This is intended for SQL-level functions written in C for cases when
they want to return an input parameter unmodified; NULL if that parameter
is NULL, or the exact value of the parameter otherwise. The return type
and the given input parameter must have matching types. The caller must
verify whether it's safe to not copy non-byval types and not de-toast
variable-length typed inputs.
Here we introduce two usages of the new macro for the transition
functions for sum(int4) and sum(int2). These just need to return the
int8 aggregate state value unmodified when asked to transition a NULL
input; the aggregate state may or may not be NULL at that point.
PG_RETURN_ARG() is coded to work for either case.
This is primarily being added to partially circumvent an argument
against adding overflow checks on sum(int2) and sum(int4), which a pending
patch proposes to do. Effectively, here we're removing 1 jump per
transition function, and that commit plans to add a new jump, so this
commit allows a net zero increase in jumps for both of those functions.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAApHDvp-3v1OWbo7RavBf37=MjNScqhj4vyAyHOUDY51nGUnog@mail.gmail.com
ruleutils.c's looks_like_function() assumed that all COERCE_SQL_SYNTAX
cases produce output that looks like a function call (or at least
like one of the syntaxes the func_expr_windowless production accepts).
But AT TIME ZONE, AT LOCAL, OVERLAPS and IS xxx NORMALIZED print as
other sorts of a_expr, so those cases would produce invalid SQL syntax
when used in a FROM clause. Extend the logic to detect which specific
functions are safe or unsafe.
Author: Henson Choi <assam258@gmail.com>
Reviewed-by: Prashant Chinnam <crprashant@hotmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAAAe_zC68s-DwVX1o7C1HVcRk+Z+RTS6Vw19auZFfz=KaX8tZA@mail.gmail.com
Backpatch-through: 14
8f29467c5 removed the hash_mem cap from the nbuckets calculation in
BuildTupleHashTable() and put the trust into the planner to provide that
function with a sane value, which it would only cap at 2^32. There are
various places in the planner which cannot provide good estimates here
and it's been demonstrated that it's not difficult to craft a query
which requests 2^32 buckets.
Here we reintroduce the sanity checks so we don't end up making the hash
table's initial size to something that would exceed hash_mem. This may
cause the hash table to have to grow several times before it fills, but
that seems much better than having the executor OOM due to the request
being too large. Users are also used to work_mem and
hash_mem_multiplier having an effect on query performance, so this
shouldn't be too surprising. It also restores the limits that were
imposed pre-8f29467c5, which were tried and tested.
Bug: #19759
Reported-by: Alperen Keles <alpkeles99@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/19759-acd3f33402e54f36@postgresql.org
Backpatch-through: 19
Commit df1be4ddc8 made promotion skip waiting for the barrier that
refreshes each process's cached logical WAL logging state in some
cases, but promotion still waits for it on a standby with wal_level =
replica that has a valid logical slot. A backend blocked sending data
to a client that has stopped reading can't process the barrier, so
promotion can wait for a long time.
Add a note about this case in the failover section, along with ways to
get out of the wait and to bound it in advance.
Backpatch to v19, where commit 67c20979ce introduced this wait.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Reviewed-by: Zhijie Hou <houzhijie22@gmail.com>
Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqToBk9c2sPAgha-kCgxJhVwn90j1sCyUXrvydq51LKAqA@mail.gmail.com
Backpatch-through: 19
This was blocked during normal CREATE TABLE, but cases such as CREATE
TABLE (LIKE) would copy the source table's NOT NULL NO INHERIT constraints
and apply them to the partitioned table being created. This could mean
that partitions didn't correctly receive the NOT NULL constraint when they
were created, thus could contain NULL values.
cacef1722 added a test to ensure CREATE TABLE (LIKE) PARTITION BY fails
when the source table has any CHECK NO INHERIT constraints. Here we do
the same for NOT NULL constraints.
It's probably unlikely that people have commonly done this, but the
release notes may wish to note that people should check if this has
happened to them. The following query can used each database to identify
problematic constraints:
SELECT conrelid::regclass, conname, connoinherit
FROM pg_constraint
WHERE contype = 'n'
AND connoinherit = true
AND conrelid IN (SELECT oid FROM pg_class WHERE relkind = 'p');
If no results are shown, the queried database isn't affected.
To resolve these issues, unwanted constraints can simply be dropped from
the partitioned table with:
ALTER TABLE ... DROP CONSTRAINT ...;
To remove the NO INHERIT flag, use:
ALTER TABLE ... ALTER CONSTRAINT ... INHERIT;
This will fail if any partition contains a NULL value in the given
column. Finding the cause of failures is made more difficult by the
planner trusting that "WHERE col IS NULL" cannot be true because of the
presence of a NOT NULL constraint. Affected users may need to resort to
tricks like WHERE col+0 IS NULL to find these. If it was thought to be a
common problem, then we could adjust get_relation_notnullatts(), and the
equivalent functions on older branches to ignore constraints marked as NO
INHERIT on partitioned tables, but currently that would require an
expensive lookup to find the pg_constraint row.
Backpatch to v18 where NOT NULL NO INHERIT constraints were added by
14e87ffa5.
Bug: #19770
Reported-by: Ann C <anncalla@163.com>
Author: David Rowley <dgrowleyml@gmail.com>
Author: Nitin Motiani <nitinmotiani@google.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Nitin Motiani <nitinmotiani@google.com>
Discussion: https://postgr.es/m/19770-7c6e75fc30df7e06@postgresql.org
Discussion: https://postgr.es/m/CAH5HC96YCzOy+9JZLQ4n0sMchVnharTOXrr4sbYuh=_d+ffjAA@mail.gmail.com
Backpatch-through: 18
pg_stat_statements decides whether a query is toplevel from the
nesting level at the time each hook runs. A QueryDesc can outlive the
level it started at. For example, a cursor's query starts at level 1,
inside DECLARE, but if the cursor is closed implicitly at COMMIT,
ExecutorEnd runs at level 0. The entry created for the query at parse
analysis has toplevel = false, so ExecutorEnd misses it and stores a
duplicate with the raw query text and toplevel = true.
The same happens with track = top when another extension such as
auto_explain requests query instrumentation. ExecutorEnd stores the
query even though ExecutorStart did not track it.
PGSS is upgraded to save the nesting level in the extension state of the
QueryDesc when ExecutorStart() tracks a query. ExecutorEnd() then uses
it in place of the current level and skips the query if no state was
saved.
For now, only the nesting level is tracked in the private extension
state attached to QueryDesc by PGSS. auto_explain is not touched
either. All these pieces are left as future improvements.
No backpatch is done, due to the ABI changes required in QueryDesc.
This relies on the work done previously in e5d5d1ad1e.
Reported-by: Gaurav Singh <gaurav.singh@yugabyte.com>
Author: Sami Imseih <samimseih@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAEcQ1bZrgwKsauC=03otebkLk77Zc_hYNjhTP3OUdpXs++u+BA@mail.gmail.com
Discussion: https://postgr.es/m/CAA5RZ0v281uwSudcFrY8BymSRDM4J6PAHfzqg_9R+i7zWbTb2A@mail.gmail.com
Extensions sometimes need to carry private data from one executor hook
to another for a given query. ExecutorEnd() can run in a different
context than ExecutorStart(), so backend-wide variables cannot be used
for that.
This commit adds a way for loadable modules to attach private state to a
QueryDesc, modeled in the same fashion as what is already done for
PlannerGlobal, PlannerInfo, RelOptInfo and ExplainState.
GetQueryDescExtensionId() maps an extension name to a backend-local ID.
SetQueryDescExtensionState() and GetQueryDescExtensionState() store and
fetch an opaque pointer for that ID. The array of pointers is allocated
in the memory context of the QueryDesc, and is freed by FreeQueryDesc().
What the pointers refer to is owned by the extensions.
There are no users of this facility yet. A follow-up commit will make
use of it. Some potential use cases have been identified for
pg_stat_statements and auto_explain, where both need to be able to
co-exist in peace. Extensions living outside of core may also find a
use for it.
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Discussion: https://postgr.es/m/CAEcQ1bZrgwKsauC=03otebkLk77Zc_hYNjhTP3OUdpXs++u+BA@mail.gmail.com
Base backups call stat() on each file while scanning the data directory,
and later send exactly that many bytes. If the file is replaced by for
example a concurrent rename(), the backup contains the new file
truncated or zero-padded to the size of the old one. This is harmless
for relation files as WAL replay repairs them. However, it causes harm
for non-relation files manipulated as temporary file renamed in place.
Here is a list of non-relation files that use a temporary file followed
by a final rename() (I may be missing some):
* postgresql.auto.conf
* pg_logical/replorigin_checkpoint (a damaged copy fails its CRC check
and prevents startup)
* pg_prewarm's autoprewarm.blocks
* pg_stash_advice.tsv.
When sending files that are not relation files, fstat() is called again
on the opened file and the refreshed stats are used for the tar header
and the backup manifest. Since the rename
is atomic, the opened file is always complete. Relation files keep using
the earlier stat, to avoid copying data that WAL replay would restore
anyway (because that's cheaper).
Back-patch down to v15. v14 is going to be EOL soon, so I have decided
to leave it out of the picture (risk/reward thing).
Author: David Steele <david@pgbackrest.org>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Shihao Zhong <zhong950419@gmail.com>
Discussion: https://postgr.es/m/CAGRkXqS_On-pLgcv6xjH8mvGhdcPmfWtythW3r8pYKUG+hsiuQ@mail.gmail.com
Backpatch-through: 15
heapam_index_validate_scan() reports PROGRESS_SCAN_BLOCKS_DONE as the
number of the block that the scan starts on, forgetting to count the
last block scanned.
Due to the last block not being counted, the number of blocks_done never
matched blocks_total. This information could remain on the view for a
long time, depending on the waits done between the phases of CREATE
INDEX CONCURRENTLY or REINDEX CONCURRENTLY.
This is incorrect still purely cosmetic, so no backpatch is done.
Author: Manu <manuelreyesbravo@gmail.com>
Discussion: https://postgr.es/m/179303526069.2212386.7608335332316408826@gmail.com
The documentation says that FOUND starts out false within each
PL/pgSQL function call. plpgsql_exec_function() and
plpgsql_exec_trigger() do set it so, but plpgsql_exec_event_trigger()
did not, leaving it NULL until the first statement that sets it.
Fix this by initializing FOUND to false, as the other two entry points
do. While at it, also set estate.err_text to "during function entry"
during this action and the subsequent instrumentation plugin call,
again to match the other two routines.
Back-patch to v19, but no further. It seems unlikely that any user
code is depending on the current behavior, but just in case, let's
not touch the stable branches.
Author: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAM2+6=U_=hZmgs1v1JJ-XJk2bpbfqQPP9MNRDbp5RoEsOW8xsQ@mail.gmail.com
Backpatch-through: 19
On Windows and in EXEC_BACKEND builds every backend reloads the SSL
configuration, with no previous one to fall back on. If a single
pg_hosts.conf entry failed to load there, the whole configuration was
discarded and the backend ran without SSL for every host. Fix by
keeping the failed entry without an SSL context in child processes
and failing the handshake only for connections selecting it.
Also document that SSL_passphrase_cmd_reload must be enabled on
Windows, like commit 0f4f45772c did for the GUC.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAN4CZFM-Hn7-bdKC8=xwCQiE128Y1Xao+aft=Grq5QA9OkZ9Kw@mail.gmail.com
Backpatch-through: 19
When validating a PL/pgSQL function with polymorphic arguments or
result type, we substitute a plausible concrete type for each
polymorphic type. anycompatiblemultirange was missed in both places
that do this, when multiranges were added. As a result, CREATE
FUNCTION with an anycompatiblemultirange argument failed with
"PL/pgSQL functions cannot accept type anycompatiblemultirange" when
check_function_bodies is on.
Also, a function returning anycompatiblemultirange was compiled by
the validator with an int4 result type, and since the function cache
key doesn't include the result type, a later call in the same session
with int4range arguments reused that entry and failed trying to cast
the multirange result to integer.
Map anycompatiblemultirange to int4multirange, as is done for
anymultirange, to fix both issues.
Author: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAM2+6=VEXCo7tPaxX+9_zN8SJ9DiPp2NTj_sPGN+Tb0wWTBp=w@mail.gmail.com
Backpatch-through: 14
In the perhaps-unlikely scenario where your session default settings
specify read-only, deferrable, serializable transactions, the implicit
transaction created by ProcessIncomingNotify() honored those settings
and would wait for a safe snapshot to be creatable; thereby getting
blocked by any long-running concurrent transaction. This is a bit
surprising, especially if you think you are committing a transaction
that doesn't have those properties.
There doesn't seem to be any good semantic reason for NOTIFY reporting
to wait for a safe snapshot, so create a code pathway that lets it
just get a plain snapshot, independently of transaction mode.
Given the lack of field complaints about such issues, I don't feel a
need to back-patch this.
Author: shihao zhong <zhong950419@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQ1bmVwBw2CXiyZeWWP=XkHPXLPcQ7BSg2Q5JKSvDAnTA@mail.gmail.com
The second phase of a multi-level nbtree page deletion operation marks a
"top parent" internal page fully deleted, and updates the to-be-deleted
subtree's half-dead page. The leaf page needs to have its top parent
link updated in preparation for marking the next top parent page (the
subtree page one level down) fully deleted. The second phase reads the
leaf page's sibling links and releases its lock on the leaf, relocking
it only after it has examined the target page. This created a small
window where stale information could be WAL logged.
It was just about possible for a concurrent split of the half-dead
page's left sibling page to update the half-dead page's left sibling
link. _bt_unlink_halfdead_page neglected to re-copy the leaf page's
sibling links when it reacquired a lock on that page. The atomic action
that the WAL record represents doesn't really need to change any of its
sibling links, so this shouldn't have been a problem. However, since we
use REGBUF_WILL_INIT for the half-dead leaf page's buffer, using a stale
value led to corrupted half-dead leaf pages on standbys. This bug has
been shown to cause wal_consistency_checking failures.
To fix, re-read btpo_prev (and btpo_next) from the leaf once it has been
relocked, before the links are logged. That way we'll reliably WAL log
the current leafleftsib.
In practice this bug was very hard to hit. It required a concurrent
split at just the right/wrong time, with a multi-level page deletion.
Even then, the resulting corruption on standbys only persisted until the
half-dead page was marked fully deleted -- which typically happened
immediately after the corruption was introduced.
Oversight in commit efada2b8e9, which fixed a race condition in nbtree
page deletion.
Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-Wznr2Gw5OQE9KMP_Xd92F9mV=gv7qE9Q6taAvfrKqsE5Qg@mail.gmail.com
Backpatch-through: 14
The header of an incremental file was only partially validated:
num_blocks and truncation_block_length were checked against
RELSEG_SIZE, but the block numbers stored after them were not checked
at all. A block number of 0xFFFFFFFF made
find_reconstructed_block_length() compute a reconstructed length of
zero, since it adds 1 to the block number in 32-bit arithmetic, and
reconstruct_from_incremental_file() then allocated zero-length
sourcemap and offsetmap arrays but still indexed them with the block
numbers taken from the file. In an assertions-enabled build, this
would be caught by an Assert, but otherwise it would index the arrays
out of bounds, typically crashing the process.
To fix, check the block numbers when the header is read, like the
other header fields.
Reported-by: Sajeeb Lohani working with TrendAI Zero Day Initiative
CheckPointTwoPhase() wrote each 2PC file in place, with O_TRUNC. A
crash mid-write, or a base backup copying the file at that point left an
empty or partial file that would make startup fail, most likely on an
"incorrect size of file" ERROR.
The 2PC files are now written through a temporary file that gets renamed
at the end, relying on the atomicity of the rename to make startup safe,
be it after a crash or a base backup.
Author: Shihao Zhong <zhong950419@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: David Steele <david@pgbackrest.org>
Discussion: https://postgr.es/m/CAGRkXqS_On-pLgcv6xjH8mvGhdcPmfWtythW3r8pYKUG+hsiuQ@mail.gmail.com
Backpatch-through: 14
add_child_join_rel_equivalences() built a child version of every
multi-relation EquivalenceClass member that overlapped the child
join's topmost parent relids. For a member that also references a
relation outside the join, the child version is of no use for what
these members exist for, which is matching sort pathkeys of the child
join, since such a member cannot be computed there.
The only effect of these members was on the clauses generated for
parameterized child-join paths, and it was a harmful one. Each child
join nested within a larger child join supplies its own version of
such a member, as does each rebuild of the same child join by GEQO.
These versions are all the same expression, yet
generate_join_implied_equalities() would equate them to each other,
producing redundant clauses that share one rinfo_serial and so trip
the assertion added by commit 8510bcc98. Even a lone such member
could yield a join clause that duplicates a condition already enforced
by the inner path, as seen in an existing regression test whose plan
now loses a redundant join filter.
To fix, consider only members that can be computed at the child join's
topmost parent joinrel. This matches what add_child_rel_equivalences()
has done for base relations since commit 2489d76c4.
In passing, simplify how both functions compute the child member's
relids. Now that the parent member's relids are known to lie within
the parent rel, the child member's relids are simply the child rel's.
Back-patch to v19, as with commit 8510bcc98.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/4a0d7906-14f4-4992-8815-16e17e1f6655@gmail.com
Backpatch-through: 19
get_param_path_clause_serials() handles join paths and Append paths
explicitly, and for anything else returns the ppi_serials of the
path's ParamPathInfo on the assumption that it is a baserel scan. But
Material, Memoize, Projection, Sort, IncrementalSort, Unique, Agg and
GroupingSets paths all inherit their subpath's ParamPathInfo, and that
assumption fails when the subpath is a join or Append.
This could drop a join clause for good. If a LATERAL UNION ALL
subquery references an outer relation in an expression that becomes an
EquivalenceClass member, the members get no child version of it, so
their parameterized scans cannot enforce the derived clause. The
appendrel's ParamPathInfo claims it nonetheless, and once a
Materialize sat on the Append, create_nestloop_path() dropped the
clause from the join above as already enforced, returning rows that
fail it.
To fix, make get_param_path_clause_serials() recurse through such
wrapper paths, since they enforce no clauses of their own. Also use
it in get_memoize_path(), which unsafely read ppi_serials directly for
its inner-unique check.
To try to clarify what's going on here, introduce a new function
get_wrapper_parampathinfo() that pathnode.c should use instead of
directly assuming that copying a wrapper path's subpath's param_info
is what to do. This is just cosmetic at present but might become
less so in future.
Back-patch to v16, where the serial-based detection of enforced
clauses was introduced.
Author: Richard Guo <guofenglinux@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49jVFRZ7oOgMK9zYt7d=SLxpiz=dN=QMu5iV8TS+9T0AA@mail.gmail.com
Backpatch-through: 16
HeapTupleSatisfiesVacuumHorizon() did not set HEAP_XMAX_INVALID on a
tuple inserted, then updated by a subtransaction that rolled back.
rewriteheap.c relies on this hint to tell if such tuples should be
treated as updated and would drop them, causing rewrites to lose some
tuples.
HeapTupleSatisfiesVacuumHorizon() is updated to set the hint
HEAP_XMAX_INVALID, so as heap AM is able to redirect such a tuple to its
correct route, and does not lose it.
This issue impacted both CLUSTER (down to v14) and REPACK [USING] (down
to v19).
Reported-by: Alperen Keles <alpkeles99@gmail.com>
Author: Shihao Zhong <zhong950419@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reported-by: Zhijie Hou <houzhijie22@gmail.com>
Reported-by: Takeshi Ideriha <iderihatakeshi@gmail.com>
Reported-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/19760-8f9a0eba17bc8c92@postgresql.org
Backpatch-through: 14
Late-model clang is able to notice, when building posix_sema.c with
USE_NAMED_POSIX_SEMAPHORES not defined, that the static variable
nextSemKey is set once and read nowhere. Rearrange the #ifdef's
to not declare or set it unless USE_NAMED_POSIX_SEMAPHORES. As a
small side benefit, we can skip a stat() call altogether if we're
not going to need it to initialize this variable, which I think
clarifies the code a fraction.
Per buildfarm member fritillary.
Commit 282b1cde9 changed pg_listening_channels() to keep a hash table
scan open between calls of this set-returning function. If the
executor stops fetching rows early, as with a target-list call under
LIMIT, the scan remains registered until transaction end, resulting in
leak complaints and eventually a PANIC.
Instead materialize the result in a tuplestore, so that we can
complete the hash scan within a single call.
Author: Joel Jacobson <joel@compiler.org>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/069fc976-bee1-4f0f-b22b-86ef7ed4f9ff@app.fastmail.com
Backpatch-through: 19
This is part of the overall move toward meson. This particular step
was previously held up because the headerscheck target was not ported
to meson, but this has been done a few months ago (commit
e82fc27e09). This conversion is relatively straightforward. The
scope of each individual build step (e.g., what features it enables,
what optimization level it uses) has been maintained, except as noted.
It actually covers a bit more now, as it also compiles various test
code via the testprep target.
One thing we're losing is the sharing of the configure cache between
the steps (gcc.cache, clang.cache). There is a way to do that in
meson (meson-private/coredata.dat), but it's not meant to be used
across different build directories, and so making that work might be a
more significant restructuring. The overall performance of the CI
runs seems to be about on par nonetheless.
Bitcode for JIT inlining is no longer built in this job, since meson
doesn't support it. It added little to warning coverage, and the
Linux autoconf job still builds it.
Co-authored-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/e52e2c29-4834-4c64-a200-eddcba38fb4d%40eisentraut.org
Commit 8b08f7d482 (pg11-era), for no apparent reason, made
validatePartitionedIndex() release the locks on the parent partitioned
index and table when marking the parent index valid after marking the
index partition as valid. This leads to race condition, which
particularly manifest when restoring from pg_dump in parallel mode, for
instance causing restore of FKs to fail if the PK that they depend on is
being concurrently validated. Fix by retaining those locks until
transaction commit, as we normally do everywhere in DDL code.
Per buildfarm failures.
Author: Andrey Borodin <x4mmm@yandex-team.ru> (in absentia)
Diagnosed-by: Kirill Reshke <reshkekirill@gmail.com>
Backpatch-through: 14
Discussion: https://postgr.es/m/l4joqljvbou26unhplkdj7m5olxn7rsy2loqlcta44nie5k3bs@zudmva7han7g
A jsonb_ops index scan for 'strict $ == "a scalar"' missed a matching
root string. Root scalars are stored in single-element arrays, so
gin_extract_jsonb() indexes strings with JGINFLAG_KEY. In strict mode,
jsonb_ops__extract_nodes() instead searches for JGINFLAG_STR at the
root. Leave gin_extract_jsonb() alone (else we'd force people to
reindex) and make jsonb_ops__extract_nodes() search for JGINFLAG_KEY
instead.
Author: Rui Zhao <zhaorui126@gmail.com>
Reviewed-by: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAHWVJhHH0k3PxWmS==V1p_PONOMUaSm6LjUV487fwnOqd0jTQw@mail.gmail.com
Backpatch-through: 14
Commit 1811f1af9 put outer tuples with null join keys into a
tuplestore and emitted them at the end of the batch, which risks
expensive spills to disk if there are a lot of them. It's difficult
to avoid the tuplestore in a multi-batch parallel join. But in the
non-parallel and single-batch cases we can just emit such tuples
immediately, at the tiny cost of propagating the "is-null" result
flag one call level further up. This avoids a performance regression
from v18 for such cases.
Author: shihao zhong <zhong950419@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqTgugmYrtnP_11NjL_GCXwAfzVfm5WKUNFsfZOzxDF2rA@mail.gmail.com
Backpatch-through: 19
I added these assertions in an attempt to show that setting
ret_old_alias/ret_new_alias in set_deparse_plan() fully covered
all the parts of the plan where RETURNING Vars could be found.
But it emerges that the returningOldAlias/returningNewAlias
fields they are filled from can sometimes be NULL themselves,
allowing the asserts to fail. We discussed various ways that
we might adjust the rewriter to prevent that, but the upshot
of the discussion was just to remove the asserts again.
Reported-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWWsORQbyViXmsvT4-NWkjuykkjjk2c+gQtq+vw2PJON-A@mail.gmail.com
Backpatch-through: 18
GetJsonBehaviorConst() builds Const values for clauses like ON EMPTY or
ON ERROR in SQL/JSON, and was setting up a location without considering
the fact that these are not constants as per the query text: they are
query keywords. For example "EMPTY ARRAY ON EMPTY" became "$2 ARRAY ON
EMPTY", leading to confusing information reported for example in PGSS.
The Const nodes used by GetJsonBehaviorConst() is now left unset. The
location was only used for an error position. This code path cannot be
reached currently but we cannot use the expression location if not set;
this spot is switched to the location of behavior keyword instead, which
is better than nothing.
This is obviously a bug. Unfortunately, no backpatch is done as this
impacts the information of saved in PGSS and the shape of the Nodes
themselves.
Oversight in 6185c9737c.
Author: Ilya Staroverov <i.staroverov@ftdata.ru>
Reviewed-by: Sami Imseih <samimseih.pg@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/457969A9-5D24-422F-B09E-F388053DE028@localhost.localdomain
Buildfarm animal trilobite turned red because the test that is
being removed crashes due to a long-standing bug in how
ri_FetchPreparedPlan() handles an invalid SPI plan, which the test
started reaching after 6aecabcd5e made it go through the SPI path
and which CLOBBER_CACHE_ALWAYS exposes. Fixing that bug will take
some time, so in the meantime remove the test case so the buildfarm
can turn green and other committers are not distracted by this.
The test will be re-added with the commit to fix the bug.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reported-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/3043944.1792163844@sss.pgh.pa.us
Discussion: https://posgre.es/m/CA+HiwqE=QFc7xS72xZ5scFNUbLdOCJyvew+3toLebsWipJOqnQ@mail.gmail.com
Backpatch-through: 19
tsqueryrecv() calls findoprnd() even when the input has no query
items, so it fails on an empty tsquery, which is legal data.
This breaks, for example, binary COPY of a table containing an
empty tsquery.
Skip the operand walk for an empty query, as parse_tsquery() does.
Bug: #19778
Reported-by: Shay Rojansky <roji@roji.org>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19778-1befc1fba593ab1b@postgresql.org
Backpatch-through: 14
Commit a9c350d9e stopped filling in the new schema's name, and the
partial revert in 962e0a723 kept that, which was an error: it allows
names that will be looked up rather than created to be resolved in
pg_temp or pg_catalog, which they should not be. This for example
allowed a CREATE INDEX to target a chance-match table name in pg_temp
rather than the expected table in the new schema. Revert to the old
approach of putting the new schema's name into each RangeVar. We can
still keep the refactorization of dealing with RangeVars as such
instead of working with char* pointers.
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqRMkfs25+7ReRkF4WCogEnD1k1=L=fmA_kxbtDfmhFn-g@mail.gmail.com
Backpatch-through: 19
asyncQueueReadAllNotifications() sets our isAdvancing flag and clears
it when done. But if we error out in between, for example on a
cancel while waiting for a safe snapshot, the flag stays set and
SignalBackends() never advances or wakes us again, so the queue tail
stops moving. To fix, clear the flag in AtAbort_Notify().
(We need not worry about this in subtransaction abort, since
asyncQueueReadAllNotifications() is only called in top-level
transactions.)
Author: shihao zhong <zhong950419@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAGRkXqQ1bmVwBw2CXiyZeWWP=XkHPXLPcQ7BSg2Q5JKSvDAnTA@mail.gmail.com
Backpatch-through: 19
readTimeLineHistory() previously parsed WAL switchpoints in timeline
history files using sscanf("%X/%08X") into uint32 values. An
overlong high component could overflow, while an overlong low component
was truncated to its first eight digits. Trailing characters were also
ignored.
Extract the switchpoint token and use pg_parse_lsn() to validate and
convert it, requiring one to eight hexadecimal digits per component
with no trailing characters, as for pg_lsn input. This matches the
change to pg_rewind's history file parser in commit f31d6fbc31.
Author: Jan Nidzwetzki <jan@planetscale.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/8b1abbd0-7335-4e68-93ec-91cdbbfd9568@planetscale.com
A new test file is added to track how the various SQL/JSON query
patterns fare in PGSS, as allowed by the parser:
- Query functions
- JSON_TABLE
- Constructors and predicates.
- etc.
This will be handy for the evaluation of some follow-up patches that aim
to tweak the query jumbling for a portion of some of these SQL/JSON
queries. Some of them are currently slightly broken, tracking as
parameters what should be a keyword, and we never knew about them.
Until now.
The author has written most of the patch. I have noticed a couple of
gaps and some inconsistencies upon further review, leading to a result
more consistent with the usual approach used by the PGSS tests.
Author: Ilya Staroverov <i.staroverov@ftdata.ru>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/457969A9-5D24-422F-B09E-F388053DE028@localhost.localdomain
libpqwalreceiver parses the WAL flush location returned by
IDENTIFY_SYSTEM using sscanf("%X/%X") into uint32 values. This
accepts overlong components and trailing characters, potentially
producing a different LSN from the one supplied by the upstream server.
Use pg_parse_lsn() instead, requiring one to eight hexadecimal digits
per component and no trailing characters, as for pg_lsn input. Reject
malformed locations with the existing protocol violation error.
Suggested-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Jan Nidzwetzki <jan@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/tencent_C6ED19DF1759CDD37E786F7EE0B5207D0009@qq.com
The replication command scanner accepts arbitrarily long hexadecimal
LSN components, but converts them with sscanf("%X/%08X") into uint32
values. An overlong high component can overflow, while an overlong low
component is truncated to its first eight digits. This can cause
START_REPLICATION to use a different WAL location from the one requested.
Use pg_parse_lsn() to validate and convert the complete LSN token,
requiring one to eight hexadecimal digits per component, as for pg_lsn
input. This applies to both physical and logical replication.
Reported-by: ♂π≌26218 <1991230470@qq.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Jan Nidzwetzki <jan@planetscale.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/tencent_C6ED19DF1759CDD37E786F7EE0B5207D0009@qq.com
The function is declared in pg_proc.dat a bigint as its second input
argument, but the function's code used PG_GETARG_INT32().
This is an issue in 32-bit architectures in stable branches (up to v18)
where bigint is still passed by reference, reporting bogus numbers.
Since 2a600a93c7 (v19 and HEAD), this is less of a problem as Datums
became 8-byte wide everywhere.
Oversight in ab0dfc961b.
Author: Markos Fountoulakis <markos@planetscale.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/e791bbb7-af3a-4a91-a5bb-c744e5c3ef54@planetscale.com
Backpatch-through: 14
Fujii Masao