If a "char" column has a statistics histogram, scalarineqsel()
would fail with "cache lookup failed for collation 0". Avoid
the failing lookup by acting as though the collation is "C".
Prior to commit 06421b084, this code didn't fail because
lc_collate_is_c() intentionally didn't spit up on InvalidOid.
It did act differently though: it would take the non-C-collation
code path and hence apply strxfrm using libc's prevailing locale.
But that seems like the wrong thing for a non-collatable comparison,
so let's not resurrect that aspect.
Author: Feng Wu <wufengwufengwufeng@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CACK3muq6s-O1Wc3w4dRL1Fe8YQ-Fz1zJbezeQwhuLgNxGNEFiA@mail.gmail.com
Backpatch-through: 18
Replace the VARIADIC text[] alternating key/value option interface with
typed named boolean parameters for pg_get_role_ddl(), pg_get_tablespace_ddl(),
and pg_get_database_ddl(), as added by commit 4881981f92 and friends.
The new signatures are:
pg_get_role_ddl(role regrole,
pretty boolean DEFAULT false,
memberships boolean DEFAULT true)
pg_get_tablespace_ddl(tablespace oid/name,
pretty boolean DEFAULT false,
owner boolean DEFAULT true)
pg_get_database_ddl(db regdatabase,
pretty boolean DEFAULT false,
owner boolean DEFAULT true,
tablespace boolean DEFAULT true)
This provides type safety at the SQL level, allows named-argument calling
syntax (pretty => true), removes the runtime string-parsing machinery
(DdlOption, parse_ddl_options) in favour of direct PG_GETARG_BOOL() calls,
and allows the functions to be marked STRICT.
While we're here, I added an extra TAP test for pg_get_database(owner =>
false, ...)
Catalog version bumped.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Discussion: https://postgr.es/m/DHM6C7SLS4BN.1WW9Z4PRPN0VJ@jeltef.nl
(and on Discord)
The ALTER TABLE documentation said that FOREIGN KEY and CHECK
constraints may be altered, but did not distinguish between
deferrability and enforceability attributes.
Clarify that deferrability attributes can currently be altered only for
FOREIGN KEY constraints, while enforceability can be altered for both
FOREIGN KEY and CHECK constraints. Also document that setting a
constraint to ENFORCED verifies existing rows and resumes checking new
or updated rows.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/E74C57FA-1DD0-4C8E-8FB1-538034752592@gmail.com
Discussion: https://postgr.es/m/711B1ED3-1781-4B6C-A573-B58AF20770E5@gmail.com
Update the table inheritance documentation to mention not-null constraints
alongside check constraints where inherited constraints are discussed.
Also clarify that some properties of inherited constraints can now be altered
directly on child tables, while the resulting constraint must remain compatible
with its inherited parent constraints. For multiple inheritance, say explicitly
that when a column or constraint is inherited from more than one parent, the
stricter definition applies.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://postgr.es/m/E74C57FA-1DD0-4C8E-8FB1-538034752592@gmail.com
Disallow marking an inherited CHECK constraint as NOT ENFORCED when an
equivalent parent constraint remains ENFORCED. This prevents ALTER
CONSTRAINT from producing a child constraint that is weaker than one of
its inherited parent definitions.
When recursively altering a CHECK constraint to NOT ENFORCED, collect the
corresponding constraints in the affected inheritance subtree and ignore
those parent constraints while checking descendants. If a descendant also
inherits an equivalent ENFORCED constraint from a parent outside the
current ALTER, keep the descendant ENFORCED by merging to the stricter
state.
This was missed in commit 342051d73b, which introduced the ability to
alter CHECK constraint enforceability.
Add regression coverage for direct child ALTER, ONLY ALTER, mixed-parent
inheritance, and a common-ancestor diamond where all equivalent inherited
constraints can be changed together.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Jian He <jian.universality@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/E74C57FA-1DD0-4C8E-8FB1-538034752592@gmail.com
When COPY TO with FORMAT json is given an explicit column list that
names all columns in a different order, the JSON output incorrectly
used the table's physical column order instead of the requested order.
This happened because BeginCopyTo() only built a restricted TupleDesc
when list_length(attnumlist) < tupDesc->natts. When all columns are
listed (just reordered), this condition was false and no projected
TupleDesc was built, causing CopyToJsonOneRow() to emit columns in
physical order.
Fix by also building the projected TupleDesc when an explicit column
list was provided (attnamelist != NIL), even if it names all columns.
Author: Baji Shaik <baji.pgdev@gmail.com>
Reviewed-by: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/CA+fm-ROd4cNKM524n6EdgtZ9xOzOHJDNv8J_9Mvr2+2t1qWSDw@mail.gmail.com
We should defer validating FDW usage until after analysis. We have to
guard against not just the topmost table, but also individual child
partitions. Added the check to CheckValidResultRel, because it is
called after looking up child partitions (accounting for pruning), but
before the FDW can run a DirectModify update, which would bypass
per-tuple executor work.
Author: jian he <jian.universality@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Paul A. Jungwirth <pj@illuminatedcomputing.com>
Discussion: https://www.postgresql.org/message-id/flat/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com
As documented in guc.h, MAX_KILOBYTES is used to cap GUC parameters that
are measured in kilobytes of memory. This way, size_t values can fit in
builds where sizeof(size_t) is 4 bytes.
Unfortunately, huge_page_size has missed this aspect, causing
calculation failures when setting this GUC to a value higher than
MAX_KILOBYTES, up to INT_MAX.
Oversight in d2bddc2500. No backpatch is done, based on the lack of
complaints.
Reported-by: Daria Shanina <vilensipkdm@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/20260626.132415.904994526137946499.horikyota.ntt@gmail.com
psql's \pset display_true/false settings, added by commit 645cb44c54,
affect normal query output, but not \crosstabview. As a result, boolean
values used anywhere in crosstab output were always shown as "t" or "f",
which is inconsistent. Change \crosstabview so that the configured
values are displayed instead.
While at it, make \crosstabview print the \pset null string, if any, in
cells for which the query produces a NULL value. Cells for which the
query produces no value continue to have the empty string. This is an
oversight in the aboriginal \crosstabview commit, c09b18f21c.
Add a regression test covering all of this.
Author: Chao Li <lic@highgo.com>
Reported-by: Chao Li <lic@highgo.com>
Reviewed-by: David G. Johnston <david.g.johnston@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Backpatch: none needed
Discussion: https://postgr.es/m/B5E6F0A5-4B48-46D0-B5EB-CF8F8CC7D07D@gmail.com
The read stream callback apw_read_stream_next_block() advances p->pos
through the block_info array. When processing the last block, it
increments p->pos to prewarm_stop_idx before returning. The callback
itself is safe because it checks bounds before accessing the array.
However, the caller assigned blk from block_info[i] at the end of the
loop body, before the loop condition was re-evaluated. When i equaled
prewarm_stop_idx, this accessed memory beyond the allocated DSM segment,
causing a segfault.
Restructure the loop to check bounds at the top and assign blk at the
beginning of the loop body, where it is always safe. This avoids the
need for an explicit bounds check at the end.
Backpatch to 18, where the bug was introduced by commit 6acab8bdbc.
Author: Matheus Alcantara <mths.dev@pm.me>
Reported-by: Glauber Batista <glauberrbatista@gmail.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Backpatch-through: 18
Discussion: https://www.postgresql.org/message-id/CAO%2B_mTQgQyTYwDh%3DU8iTnsDmOGyWsZJjUV31SmEYwmw6_xY6Bw%40mail.gmail.com
Commit 681daed931 introduced a new EXPLAIN option "IO", but the docs
did not explain what information was added to the output. Expand the
description a little bit, similarly to the other EXPLAIN options.
While at it, fix a typo in the first sentence.
Author: Tomas Vondra <tomas@vondra.me>
createPartitionTable() passed the partitioned parent's reltablespace straight
to heap_create_with_catalog(), bypassing the default_tablespace GUC fallback
that DefineRelation() applies for CREATE TABLE ... PARTITION OF. When the
parent had no explicit tablespace (reltablespace = 0), the new partition
unconditionally landed in the database default, even if default_tablespace
was set to something else; merging or splitting a set of partitions that all
lived in a non-default tablespace produced a new partition in the database
default.
Mirror DefineRelation()'s logic: take parent's reltablespace if set,
otherwise check GetDefaultTablespace() (which reads default_tablespace
and normalises pg_default / MyDatabaseTableSpace to InvalidOid). Also
add the CREATE ACL check on the resolved tablespace and the pg_global
rejection, matching DefineRelation()'s behavior.
Update the documentation for MERGE/SPLIT PARTITION to spell out the
tablespace-selection rule explicitly.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Pavel Borisov <pashkin.elfe@gmail.com>
Discussion: https://postgr.es/m/ajQTklv8QArzTp3h%40pryzbyj2023
This addresses two gaps in the documentation:
- The function uses an xid, and was not mentioned as an exception in a
section of the docs related to xid8.
- The function returns NULL if a role does not have the privileges of
pg_read_all_stats. The execution is not denied.
Author: Chao Li <li.evan.chao@gmail.com>
Author: Yingying Chen <cyy9255@gmail.com>
Discussion: https://postgr.es/m/CAGGTb65Qmtor2nJP-ATgfWpMpD2qhKrdyO7fmRbbS++nQ=vtMw@mail.gmail.com
The uuid-to-bytea cast just serializes a valid uuid datum into its
fixed 16-byte representation. It does not have an input-dependent
error path so mark its pg_proc entry as leakproof.
Oversight in commit ba21f5bf8a.
Bump catalog version.
Author: Chao Li <lic@highgo.com>
Discussion: https://postgr.es/m/1FAAF426-9205-4F53-8D3B-F2003D96EC37@gmail.com
When compiling a DECLARE section containing a union nested
inside a struct, ecpg passes a null value for struct_sizeof to
ECPGmake_struct_type. I (tgl) didn't foresee that case in
commit 0e6060790, and wrote an unprotected mm_strdup() call.
Reported-by: iMSA (via Jehan-Guillaume de Rorthais <jgdr@dalibo.com>)
Author: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/20260625114849.34b2148e@karst
Backpatch-through: 18
pg_stat_io asserts on unexpected combinations of backend type and IOOp.
These combinations were meant to help detect bugs given our current
understanding of the system -- not serve as a set of rules for what is
allowed. The autovacuum launcher scans catalog tables and may on-access
prune them. This previously wouldn't have led to any extends of the
relation, but now that on-access pruning may pin a page of the
visibility map (4f7ecca84d), scanning tables may lead to
extending the visibility map. This would cause the launcher to trip an
assert. Since there is no reason to forbid the launcher from doing
extends, remove it from the list of backend type pgstat_tracks_io_op
flags for doing IOOP_EXTEND.
Read-only catalog scans still don't let pruning set the VM; doing so
needs table AM API changes and is left for the future.
Reported-by: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CAON2xHNOyaN9MCZohhD_NL6as3QVhGA0SOn2Hyi9w6+Y-_1bFA@mail.gmail.com
Commit fb80f388f added wal_receiver_timeout as a CREATE/ALTER
SUBSCRIPTION option, but psql tab completion did not include it in the
subscription option lists.
Add wal_receiver_timeout to completion for CREATE SUBSCRIPTION ... WITH
and ALTER SUBSCRIPTION ... SET.
Author: Chao Li <lic@highgo.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/BBC5628A-63C0-4436-B8F3-90AF59BBEB73@gmail.com
In some corner cases, a new datachecksums worker could be launched
while an old one was still running. If you're really unlucky, the old
worker could set the worker_result in shared memory and mislead the
launcher to think that a newer worker invocation completed
successfully, even though it failed for some reason. That's highly
unlikely to happen in practice as it requires several race conditions
with workers and launchers starting, failing and succeeding and at the
right moments. Nevertheless, better to tighten it up.
To distinguish different worker invocations, assign a unique
'worker_invocation' number every time a new worker is launched. In
the worker, check that the invocation number matches before setting
the worker result. This ensures that the result always belongs to the
latest invocation.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/b283fbb9-298e-4953-9120-eefaf24fae20@iki.fi
Rename the 'success' field in DataChecksumState to 'worker_result'.
That's more appropriate when it's not a simple boolean.
Don't access the field after releasing the lock in ProcessDatabase().
No other process should be modifying it, but if we bother to do any
locking in the first place, let's do it right.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/b283fbb9-298e-4953-9120-eefaf24fae20@iki.fi
It might be left to an old value if the launcher was terminated while
a worker was running. launcher_exit() sends SIGTERM to the worker,
but did not clear 'worker_pid'. Clear it, to be tidy.
Also clear it in ProcessDatabase() before starting a new datachecksums
worker, to be sure we start from a clean slate. The codepath where
WaitForBackgroundWorkerStartup() returns BGWH_STOPPED but
worker_result != DATACHECKSUMSWORKER_SUCCESSFUL didn't clear it, while
all other codepaths did clear or set it.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/b283fbb9-298e-4953-9120-eefaf24fae20@iki.fi
Move DataChecksumsWorkerResult struct to the .c file. It's not used
anywhere else since commit 07009121c2 removed the injection point test
code that the comment referred to.
Mark StartDataChecksumsWorkerLauncher() as static, since it's not
called from outside the .c file. The DataChecksumsWorkerOperation
struct can then be moved into the .c file too.
Clarify the comment on StartDataChecksumsWorkerLauncher(). It said
"Main entry point for datachecksumsworker launcher process", but I
found that misleading. That description would be a better fit for
DataChecksumsWorkerLauncherMain(), which is the process's "main"
function, rather than StartDataChecksumsWorkerLauncher().
Fix comment on WaitForAllTransactionsToFinish() on postmaster death.
The comment claimed that it sets "the abort flag" on postmaster death,
but it actually just errors outs. Improve the comment to explain why
it doesn't just use WL_EXIT_ON_PM_DEATH.
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/b283fbb9-298e-4953-9120-eefaf24fae20@iki.fi
This is similar to d3bba04154, batching all the reports of this type
received since the last batch. This covers typos and inconsistencies
for the most part.
The user-visible documentation change impacts only HEAD.
Commit 4e5920e6de disallowed RESPECT NULLS/IGNORE NULLS on
non-window functions, but it also caused the parser to check for
that clause too early in some cases. As a result, calls such as a
nonexistent function with IGNORE NULLS no longer reported the more
helpful "function ... does not exist" error, and aggregate functions
used as window functions reported "only window functions accept ..."
instead of the more accurate aggregate-specific error.
This commit moves the RESPECT NULLS/IGNORE NULLS checks so that
helpful existing errors are preserved where appropriate. This restores
"function ... does not exist" for nonexistent functions, while still
reporting that plain functions are not window functions and that
aggregates do not accept null treatment.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Tatsuo Ishii <ishii@postgresql.org>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/CAHGQGwH7VY_0GkhycyYZ4czkPGL0uGzDyOxk3uuFOSRR7wFY3g@mail.gmail.com
In get_perl_array_ref(), for a PostgreSQL::InServer::ARRAY object, we
look up its "array" key with hv_fetch_string() and then inspect the
returned SV. However, hv_fetch_string() returns a NULL pointer when
the key is absent, and the code dereferenced that result without first
checking whether the pointer itself was NULL. As a result, a plperl
function returning a forged PostgreSQL::InServer::ARRAY object that
lacks the "array" key would crash the backend with a segmentation
fault.
Fix this by checking the pointer returned by hv_fetch_string() before
dereferencing it, matching how other callers in this file already
guard the result. With the check in place, such an object falls
through to the existing error report instead of crashing.
Author: Xing Guo <higuoxing@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CACpMh+DYgcnqZwQLXXuxQcehJTd7T8UmKWSLsK4mFBEp9G2ajA@mail.gmail.com
Backpatch-through: 14
ExecInitModifyTable() rebuilds the per-result-relation lists after
dropping result relations removed by initial runtime pruning. The
re-indexing was done for withCheckOptionLists, returningLists,
updateColnosLists, mergeActionLists and mergeJoinConditions, but
fdwPrivLists and fdwDirectModifyPlans were missed. As a result, a
kept foreign result relation could be handed the wrong fdw_private,
or ri_usesFdwDirectModify could be set from the wrong plan index,
leading to wrong behavior or a crash in BeginForeignModify() and in
the direct-modify path.
show_modifytable_info() had the same problem: it indexed the
plan-ordered node->fdwPrivLists with the post-pruning executor
position, so once initial pruning removed a result relation it
could read a different relation's fdw_private (often a NIL entry),
producing wrong EXPLAIN output or a crash.
Fix by re-indexing fdwPrivLists and fdwDirectModifyPlans alongside
the other lists, saving the re-indexed private lists in
ModifyTableState.mt_fdwPrivLists and reading from there in both
nodeModifyTable.c and explain.c.
Reported-by: Chi Zhang <798604270@qq.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Author: Rafia Sabih <rafia.pghackers@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/19484-a3cb82c8cde3c8fa%40postgresql.org
Backpatch-through: 18
Previously, a parameter specified in the startup packet for a physical
replication connection could encounter an error trying to perform an
ACL check for the setting.
Problem was introduced in a0ffa885e4, but no reasonable back-patchable
solution was found, so fixing only in master.
Bumps catversion.
Discussion: https://postgr.es/m/d8f8e11f06d692fff89e6be0f22732d30cf695a0.camel%40j-davis.com
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Reviewed-by: Mark Dilger <mark.dilger@enterprisedb.com>
The final parameter of an ordinary variadic function should be an
array type. CREATE FUNCTION won't accept a declaration that isn't
like that, but it's possible to put an incorrect combination into a
pg_proc.dat entry. Sadly, the opr_sanity test that was supposed to
check that is broken and does not report functions with non-array
final parameters. This allowed exactly such a thinko to sneak into
the recently-added pg_get_*_ddl() functions: their last argument
should be declared text[] but was declared text. (We'd probably
have noticed eventually, when somebody tried to actually pass a
variadic array to one of those functions. But their regression
tests do not do that.)
Fix those functions, and fix the opr_sanity test so we'll notice
next time. Bump catversion for new pg_proc contents.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/D41A334E-ED9E-42EE-830D-28D4D36E9317@gmail.com
Since d51697484, psql's scanner treats CREATE SCHEMA as a command that
may contain SQL-standard routine bodies, so that semicolons inside
BEGIN ATOMIC ... END blocks do not terminate the command too early.
However, the code counted BEGIN/END throughout CREATE SCHEMA, so that
it could be fooled by valid (and previously accepted) code such as
CREATE SCHEMA s CREATE VIEW begin AS SELECT 1;
Improve this by explicitly checking whether each CREATE sub-clause is
CREATE [OR REPLACE] {FUNCTION|PROCEDURE}, and only counting BEGIN/END
within those clauses. Since CREATE FUNCTION/PROCEDURE wasn't allowed
in CREATE SCHEMA before d51697484, this will not risk failure on any
cases that worked before v19.
There remain cases that fool the top-level CREATE FUNCTION/PROCEDURE
heuristic and thus also the CREATE SCHEMA case, for example
CREATE FUNCTION begin () ...
But that's been true all along with no field complaints, so we'll
leave that issue for another day.
In the name of keeping things readable, move the logic supporting
this out of the {identifier} flex rule and into some small new
subroutines. Also rename existing related PsqlScanState fields
to help distinguish them from the added fields.
This patch also fixes what seems to me (tgl) a small bug: \;
would reset BEGIN/END detection even when inside parens or BEGIN.
That's unlike what a plain semicolon would do, and no such effect
is suggested by the documentation.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/8E03BB8D-003D-4850-9772-5F8015A5A0C7@gmail.com
When ALTER TABLE ... ATTACH PARTITION matches partition indexes to the
parent table's indexes, invalid indexes are skipped. This commit
improves the documentation to describe what e90e9275f5 has changed:
invalid indexes are skipped, and only valid indexes are considered for a
match.
Author: Mohamed Ali <moali.pg@gmail.com>
Reviewed-by: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAGnOmWpAMaE-BOkpwM6mJnHcpS2QZ8yLSSaqmz+vryEsbCWWWA@mail.gmail.com
Backpatch-through: 14
The "identity" column of pg_identify_object() for property graph
objects can be long string of names connected by "of", e.g. "a of l of
e of g". The type of the first named object is given by column
"type". But the types of intermediate objects are not easy to find
from the identity string especially when some of them share the same
name. Some objects, like user mappings or authorization identifier
members, add types of objects other than the first one in the identity
string. Do the same for property graph objects.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/aej1DkLwhyZWmtxJ%40bdtpg
The pages of pg_dump, pg_dumpall and pg_upgrade mentioned that their
--no-statistics and --statistics options did not include the handling of
statistics created by CREATE STATISTICS, which was wrong.
Oversight in c32fb29e97.
Reported-by: Igi Izumi <igi@sraoss.co.jp>
Discussion: https://postgr.es/m/19529-c7eb1e7a0b07eae6@postgresql.org
This function called the resource-kind-specific ReleaseResource()
method for each item before deleting that item from the resowner.
That's backwards from the ordering in ResourceOwnerReleaseAllOfKind,
and it's not very safe. If ReleaseResource throws an error then the
subsequent abort cleanup will come back here and try to release that
item again, possibly leading to a double-free or similar crash,
and in any case risking an infinite error cleanup loop. This mistake
explains why the pgcrypto bug just fixed in 80bb0ebcc led to a crash
rather than something more benign.
Remove the item from the resowner, then call ReleaseResource,
matching the way things were done before b8bff07da. If there
is a problem of this sort, we'd prefer to leak the item than
suffer the other likely consequences.
Per further analysis of bug #19527.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/646741.1782157515@sss.pgh.pa.us
Backpatch-through: 17
Raising an error within a function using an OSSLCipher object led
to a complaint from ResourceOwnerForget and then a double-free crash,
because ResOwnerReleaseOSSLCipher forgot to unhook the OSSLCipher
object from its owner. (The sibling logic for OSSLDigest objects got
this right, as did every other ReleaseResource function AFAICS.)
Oversight in cd694f60d.
Bug: #19527
Reported-by: Yuelin Wang <3020001251@tju.edu.cn>
Author: Yuelin Wang <3020001251@tju.edu.cn>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19527-6e7686960c6dce78@postgresql.org
Backpatch-through: 17
When left-join removal deletes a relation, remove_rel_from_query()
updates the relid sets attached to RestrictInfos and
EquivalenceMembers, and the canonical PlaceHolderVar held in each
PlaceHolderInfo, but it does not rewrite the PlaceHolderVars embedded
in clause and EquivalenceClass member expressions. That has been
fine, because later processing consults those relid sets rather than
the embedded PlaceHolderVars.
However, such an expression may afterwards be translated for an
appendrel child and have its relids recomputed from scratch by
pull_varnos(). If the embedded PlaceHolderVar's phrels still mentions
the removed relation, pull_varnos() folds it back in, so the rebuilt
clause's relids reference a no-longer-existent relation. That yields
a parameterized path keyed on the removed relation, tripping the
Assert on root->outer_join_rels in get_eclass_indexes_for_relids().
Fix by stripping the removed relids from the PlaceHolderVars in
surviving rels' baserestrictinfo and in EquivalenceClass member
expressions, keeping them consistent with the canonical
PlaceHolderVars.
This is only reachable on v18 and later, where
match_index_to_operand() began ignoring PlaceHolderVars; before that,
the wrapping PlaceHolderVar prevented the index match that exposes the
stale relids.
Reported-by: Alexander Kuzmenkov <akuzmenkov@tigerdata.com>
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/CALzhyqwryL2QywgO03VQr_237Sq3MEVgTTT2_A9G3nGT5-SRZg@mail.gmail.com
Backpatch-through: 18
PL/Python set-returning functions can crash with a use-after-free when
CREATE OR REPLACE FUNCTION is executed while the SRF is mid-iteration.
The crash occurs because srfstate->savedargs is allocated in proc->mcxt,
which gets deleted when the procedure is invalidated, leaving a dangling
pointer that PLy_function_restore_args() then dereferences.
The best fix is to use reference counting to prevent destroying the
function state while it's still in use, similar to what PL/pgSQL has
done. Rather than inventing a new wheel, this commit converts
PL/Python to use the funccache.c infrastructure.
The main challenge is that PL/Python uses SFRM_ValuePerCall for SRFs,
where the handler is called multiple times. A naive implementation
would allow the refcount to return to zero between calls, but we need
to hang onto the original state and function body. SQL-language
functions face the same challenge, so this commit follows the same
approach used in functions.c: maintain a per-call-site cache struct
(PLyProcedureCache) in fn_extra that holds both the pointer to the
long-lived PLyProcedure and the SRF execution state.
The use_count is incremented when we first obtain the procedure and is
decremented via a MemoryContextCallback registered on fn_mcxt, which runs
even during error aborts. Cleaning up the per-call SRF state needs more
care: an ExprContextCallback handles the in-query cases, since the
iterator is not guaranteed to run to completion (for example a LIMIT or a
rescan can abandon it early). But unlike SQL functions, whose resources
are released by transaction abort, PL/Python holds Python reference counts
on the iterator and saved arguments that abort will not release, and
ExprContextCallbacks are not invoked during an error abort. The
MemoryContextCallback on fn_mcxt therefore doubles as the backstop that
releases those references when a query errors out mid-iteration.
Since fn_extra is now used for PLyProcedureCache, this commit removes
use of the funcapi.h SRF infrastructure (SRF_IS_FIRSTCALL,
SRF_RETURN_NEXT, etc.) and switches to direct isDone signaling via
ReturnSetInfo, matching how SQL functions handle ValuePerCall mode.
This fixes a longstanding bug, so ideally we'd back-patch it. But
it'd be impractical to back-patch further than v18 where funccache.c
came in. The patch is somewhat invasive, and the bug only arises in
very uncommon usages (which is why it evaded detection for so long).
On the whole, the risk/reward ratio for putting this into v18 doesn't
seem good, so commit to master only.
Bug: #19480
Reported-by: Andrzej Doros <adoros@starfishstorage.com>
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19480-f1f9fdce30462fc4@postgresql.org
The documentation for pg_get_sequence_data() did not match the
function's behavior. It stated that either USAGE or SELECT privilege
was sufficient, but the function returns sequence data only when the
caller has SELECT privilege.
The documentation also did not explain that the function returns a row
containing all NULL values when sequence data cannot be returned, such
as when the sequence does not exist or the caller lacks the required
privilege.
Update the documentation to reflect the actual behavior, including the
required privilege and the result returned when sequence data is
unavailable.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Tristan Partin <tristan@partin.io>
Discussion: https://postgr.es/m/CAHGQGwGNTaXnBKUV510_P1KwhdbHT+kgZ4zU5njBHy7nCqdhzg@mail.gmail.com
When synchronizing sequences for logical replication, a
publisher-side permission failure could be reported as if the sequence
were missing on the publisher, making the real cause harder to
identify.
This happened because pg_get_sequence_data() returns a row of NULL
values when the replication connection lacks permission to read a
sequence. Sequence synchronization treated that the same as a missing
sequence, causing it to emit a misleading "missing sequence on
publisher" warning.
Fix this by distinguishing permission failures from genuinely missing
sequences. The synchronization query now checks whether the
replication connection has the required privilege for each published
sequence, allowing the worker to report permission failures
separately.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Tristan Partin <tristan@partin.io>
Discussion: https://postgr.es/m/CAHGQGwGNTaXnBKUV510_P1KwhdbHT+kgZ4zU5njBHy7nCqdhzg@mail.gmail.com
An out-of-memory failure while initializing the type cache hash tables
would issue an ERROR and leave a backend in a partially inconsistent
state. Without assertions, the server would crash with a NULL pointer
dereference on initialization re-entry when doing a type lookup due to
one or both hash tables missing. An assertion would trigger if these
are enabled in the build.
This commit changes the ordering of the type cache initialization to
become more robust on re-entry after an in-flight allocation failure:
- The two hash tables are initialized first, and can only be initialized
once.
- The initialization is considered as done once the in-progress list is
allocated in the CacheMemoryContext. This is now the last allocation
step.
- Last, the callbacks are registered. These can only fail with a FATAL
error, taking down the process so leaving the process in a non-complete
state is fine.
This is in the same spirit as b85f9c00fb and 29fb598b9c, where
random allocation failures can make the backend go crazy in the code
paths fixed due to the static states becoming inconsistent. Like the
other fixes, this is unlikely going to show up in practice, so no
backpatch is done.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/e77acaac-a1b3-40b3-99ee-5769b4e453e4@gmail.com
In StandbyReleaseXidEntryLocks, a failure in acquiring a lock with
LockAcquire() due to an out-of-memory problem would lead to an
inconsistency with the lock state cached in the startup process,
impacting the list of RecoveryLockXidEntrys. The code is updated here
so as the cached state is updated once the lock is acquired.
This problem is unlikely going to happen in practice. Even if it were
to show up, it would translate to a LOG message for non-assert builds
(assertion failure otherwise), so no backpatch is done. This commit is
in the same spirit as 29fb598b9c, with a problem emulated by injecting
random failures for allocations.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Discussion: https://postgr.es/m/e77acaac-a1b3-40b3-99ee-5769b4e453e4@gmail.com
pg_mkdir_p creates each missing path component with a stat() followed
by mkdir(). If the stat() reports the component as absent but another
process creates it in the window before this process's mkdir(), mkdir()
fails with EEXIST and pg_mkdir_p treated that as a hard error -- unlike
"mkdir -p", which is meant to be idempotent and race-tolerant.
This shows up when several processes concurrently create paths that
share an ancestor directory: for example, parallel initdb runs whose
data directories live under a common temporary directory. One process
wins the race to create the shared ancestor and the others fail with
could not create directory "...": File exists
Fix this race condition by first trying mkdir() and only attempting
stat() if it fails with EEXIST.
On Windows, there's an additional problem: stat() opens a file handle
and participates in share-mode locking, which means it can transiently
fail on a directory another process is concurrently creating. Use
GetFileAttributes() instead: it requests only FILE_READ_ATTRIBUTES
and is exempt from share-mode denial, so it reliably sees a
concurrently-created directory.
I (tgl) also chose to back-patch 039f7ee0f's effects on this function,
so that pgmkdirp.c remains identical in all live branches.
Author: Andrew Dunstan <andrew@dunslane.net>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3ca004de-e49b-4471-b8aa-fd656e70f68c@dunslane.net
Backpatch-through: 14
The JIT deforming code contains an optimization that determines which
columns are guaranteed to exist in the tuple. That's used to allow
skipping of reading the tuple's natts when the code only needs to deform
attributes that are guaranteed to always exist in all tuples. 83ea6c540
missed updating this code to account for VIRTUAL generated columns.
These are stored as NULLs in the tuple, but may be defined as NOT NULL.
This could result in the code thinking more columns are guaranteed to
exist than actually do.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Backpatch-through: 18
Discussion: https://postgr.es/m/1151393.1781734980@sss.pgh.pa.us
The cost parameters for the data checksums worker can be updated by the
user issuing a repeated enable checksum command, but the comments on the
struct members hadn't been updated to reflect this and were out of date.
Another part of the same comment needed better wording to be readable.
Also wrap the reading of the parameters in a lock, there is no live
bug due to not using a lock but it's still the right thing to do.
Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reported-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/2176020b-ecbc-438b-9fc3-9c3593d9e6fc@iki.fi
Newer gcc warns that this "actual_arg_types" variable may be used
uninitialized, but visual inspection indicates there's no bug. To
silence the warning, initialize the variable to zeros.
Bug: #19485
Reported-by: Hans Buschmann <buschmann@nidsa.net>
Tested-by: Erik Rijkers <er@xs4all.nl>
Tested-by: Hans Buschmann <buschmann@nidsa.net>
Reviewed-by: Tristan Partin <tristan@partin.io>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/19485-2b03231a775756f1%40postgresql.org
Discussion: https://postgr.es/m/6c52a1a6612948519468d46cb224a8c4%40nidsa.net
Add CHECK_FOR_INTERRUPTS() to the while loop in plperl_to_hstore()
that dereferences chains of Perl references, so that a circular
reference (e.g. $x = \$x) can be cancelled by the user instead of
spinning indefinitely. (We looked at detecting such circular
references, but it seems more trouble than it's worth.)
This is a follow-up to da82fbb8f, which fixed the same issue in
SV_to_JsonbValue() in jsonb_plperl.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TPbjkzUk4qJ5dHvDNEz0hBuFue3A-XWz_=897z+BC+z8A@mail.gmail.com
Backpatch-through: 14
