pg_restore_attribute_stats() and pg_restore_extended_stats()
(expressions) can handle a STATISTIC_KIND_BOUNDS_HISTOGRAM value, but
did not check its shape when importing, especially regarding:
- Empty ranges.
- Unsorted elements.
These properties are respected by ANALYZE in compute_range_stats(), when
computing a histogram for a [multi]range, and by the planner when
reading the data from the stats catalogs. The effects of importing data
with these properties depend on the compilation options:
- A assertion would be hit, if enabled.
- A non-sensical value that would make the load of the stats fail.
- A failure is hit if an empty range is loaded.
While the owner of the table or the one with MAINTAIN rights is
responsible for the stats data inserted, buggy data makes little sense
if their load is going to fail. This commit adds a validation step to
match what compute_range_stats() expects.
This issue would be unlikely hit in practice, so no backpatch is done.
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CAON2xHNM809WLR_g0ymKgU-tWxtbyH1Xvh4fqzRqy9YP2A59pg@mail.gmail.com
Explain more accurately how REFRESH SEQUENCES differs from REFRESH
PUBLICATION in ALTER SUBSCRIPTION, and note that CREATE SUBSCRIPTION uses
copy_data = true (the default) to copy initial sequence values.
Author: Peter Smith <smithpb2250@gmail.com>
Author: Amit Kapila <amit.kapila16@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAHut+PtFkGvZNihGRDoghWNKMfJufEpR9+thbG_8qPQ7RyVN4w@mail.gmail.com
We've long had enable_hashagg to discourage hashed aggregation, but
there was no equivalent for grouping that works by reading presorted
input. That's handy when investigating planner cost misestimates, and
as an escape hatch when a sorted grouping plan is chosen over a much
cheaper hashed one.
enable_groupagg (on by default) covers the GroupAggregate and Group
nodes, the sort-based Unique step used for DISTINCT and semijoin
unique-ification, and the sorted mode of SetOp. It isn't a hard
switch; it only bumps disabled_nodes, so plans with no other choice
are still produced.
Several regression and module tests had been setting enable_sort off,
and one enable_indexscan off, only to force a hashed plan. Use
enable_groupagg there instead, where that was the real intent. In
union.sql this also lets us test the hashed UNION path, which we had
no way to reach before.
Author: Tatsuro Yamada <yamatattsu@gmail.com>
Co-authored-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAOKkKFvYHSEsFazkrf9bRH14p-H27XMaqbZfRYjS6EHBruvZMQ@mail.gmail.com
Add pg_stat_progress_data_checksums to the progress reporting summary
and the list of commands with progress reporting.
Also clarify that the view reports both enabling and disabling data
checksums, and correct the documented types of its progress counters
to bigint.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAHGQGwHJHJYAkYZBi3_O13np-Rou9UL637=hB3Y_-qdCgcZn-w@mail.gmail.com
Backpatch-through: 19
I had previously made "seed" a uint64, as that's what pg_prng_seed()
expects to get. However, GetCurrentTimestamp() and PG_GETARG_INT64()
both return int64, so there was a mismatch in signedness.
There are no bugs being fixed here, but it seems cleaner to make the
variable int64 and cast to unsigned when passing to pg_prng_seed(). This
means we no longer have to use the INT64_FORMAT specifier on the unsigned
type to format the seed of the failing test correctly to allow a user to
use the same seed from SQL when trying to recreate any failures manually.
The most important thing to ensure is correct here is that users can
specify the full range of seed values that could be automatically
selected by GetCurrentTimestamp() so that we can recreate any failures
from runs where the seed was auto-selected. This still works as
expected when using the int64 type.
In passing, adjust a comment that was a little misleading.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/08fa9f01-373e-4cb4-9650-ad20517e2de3@eisentraut.org
The new partitions built for ALTER TABLE ... SPLIT PARTITION and
ALTER TABLE ... MERGE PARTITIONS are created at the explicit request of
the user, just like a plain CREATE TABLE. createPartitionTable() passes
is_internal=true to heap_create_with_catalog(), while createTableConstraints()
does the same to StoreAttrDefault() and AddRelationNewConstraints().
Pass is_internal=false in all these places instead, so that object-access
hooks treat them as user-requested objects. The is_internal flag is intended
for objects created as internal implementation details, such as a transient
heap built during CLUSTER.
While at it, pass 0 rather than PERFORM_DELETION_INTERNAL to the
performDeletionCheck() calls that pre-check the drop eligibility of the
old partitions, to match the subsequent performDeletion(). The flag has
no functional effect on performDeletionCheck(), but change this for code
consistency.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260707185751.f9.noahmisch@microsoft.com
Backpatch-through: 19
refint was sample code from the pre-built-in-FK era and has long
been documented as superseded by the built-in foreign key
mechanism. Recent fixes (see commits b0b6196386, 8cfbdf8f4d,
260e97733b, 611756948e, 1fbe2066dc, and 1541d91d1c) made it clear
that the code has more issues than its sample-code value justifies.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWUHq8Ohc6-N-xamOPYz-q3qUYMtwQX-1%3DZi%3D5N1Q_GSEQ%40mail.gmail.com
When executing an UPDATE with a RETURNING clause on a table with a
BEFORE UPDATE row trigger, the computation of the OLD values in the
RETURNING list was incorrect if the target tuple was concurrently
updated by another session, at isolation level READ COMMITTED.
The problem was that the trigger code would lock the target tuple,
waiting for the other session to commit, and then fetch the updated
target tuple, but ExecUpdate() would not realise that the target tuple
had changed, and use the outdated target tuple for computing OLD
values. Fix by having ExecUpdate() check the TM_FailureData from
trigger execution and re-fetch the target tuple if necessary.
Re-fetching the target tuple like this is a little inefficient, but
probably negligible compared to the trigger execution and update. A
better long-term fix might be to move the EPQ code out of trigger.c,
and let ExecUpdate() handle it, like ExecMergeMatched() does, but that
would likely mean changing the trigger API, which seems a bit much for
back-patching.
Backpatch to v18, where support for RETURNING OLD/NEW was added.
Bug: #19536
Reported-by: Jonas Boberg <bobergj@gmail.com>
Diagnosed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/19536-73ce5847e6c0e7b1@postgresql.org
Backpatch-through: 18
WAIT_FOR_WAL_FLUSH, WAIT_FOR_WAL_REPLAY, and WAIT_FOR_WAL_WRITE were
placed in the WaitEventClient class. But WaitEventClient is about
waiting for a socket to become readable or writable, while these events
have other delay sources as well: local fsync and local replay, which
may be disk- or CPU-bound. WaitEventIPC is a better fit, so move them
there.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260706012642.f9.noahmisch@microsoft.com
Backpatch-through: 19
When changing the expression of a generated column via ALTER TABLE
ALTER COLUMN SET EXPRESSION, objects that depend on the column via
indirect whole-row references (such as CHECK constraints, indexes)
must be handled specially, because technically pg_depend does not
contain such dependencies, see
recordDependencyOnSingleRelExpr->find_expr_references_walker.
This is a fix for commit f80bedd52, "Allow ALTER COLUMN SET EXPRESSION
on virtual columns with CHECK constraints".
Author: jian he <jian.universality@gmail.com>
Co-authored-by: Peter Eisentraut <peter@eisentraut.org>
Reported-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/flat/CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com
This moves the responsibility of setting the ProcGlobal->walwriterProc
and checkpointerProc fields into InitAuxiliaryProcess. Also switch to
the same pattern to advertise the autovacuum launcher's ProcNumber,
replacing the ad hoc av_launcherpid field in shared memory. This can
easily be extended to other aux processes in the future, if other
processes need to find them.
Switch to pg_atomic_uint32 for the fields. Seems easier to reason
about than volatile pointers. There was some precedence for that, as
were already using pg_atomic_uint32 for the procArrayGroupFirst and
clogGroupFirst fields, which also store ProcNumbers.
Todo: We could also replace WalRecv->procno with this, but that's a
little more code churn so I left that for the future.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/818bafaf-1e77-4c78-8037-d7120878d87c@iki.fi
When pg_dump or pg_restore --jobs N is interrupted with Ctrl-C on
Windows, we cancel all queries, but we don't want the cancellations to
be reported as errors to the user in the short time before the whole
process exits. That was previously achieved by calling
TerminateThread() on each worker thread before sending the cancel
message, but that doesn't appear to be 100% safe: the implementations
of write() and the socket calls inside PQcancel() might acquire user
space locks that were held by the terminated threads. (write()
certainly does that.)
Instead of silencing the threads in such a sketchy way this now sets a
volatile flag before sending any cancel requests that tells the
threads to not log errors anymore. (Instead of a volatile, it would be
better to use an atomic operation here, but that has to wait until we
add support for atomics on the frontend.)
Note that this also stops using pg_fatal() and exit to exit() from
workers on failure and instead use pg_log_error combined with
exit_nicely. If a query fails in a worker we want it to kill the
worker not the whole process. On Unix that's currently the same thing,
but on Windows workers are threads.
Author: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Bryan Green <dbryan.green@gmail.com>
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/DJPQS3FYSD4U.3DBTXA6U8IQ0Q@jeltef.nl
Effectively, a function to bitshift members by the specified number of
bits. We have various fragments of code doing this manually with a
bms_next_member() -> bms_add_member() loop. We can do this more
efficiently in terms of CPU and memory allocation by making a new
Bitmapset and bitshifting in the words of the old set to populate it.
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Greg Burd <greg@burd.me>
Discussion: https://postgr.es/m/CAApHDvq=eEdw2Qp+aSzSOtTSe+h0fnVQ55CcTNqBkLDYiRZmxw@mail.gmail.com
Sequence synchronization reports insufficient privileges on publisher
and subscriber sequences, but the warnings do not indicate which role
needs which privilege. This makes common configuration mistakes harder
to diagnose.
Add HINT messages for these warnings. Publisher-side warnings suggest
granting SELECT to the role used for the replication connection.
Subscriber-side warnings suggest granting UPDATE to the subscription
owner when run_as_owner is enabled. Otherwise, the worker runs as the
sequence owner, so no useful GRANT hint can be provided.
Suggested-by : Amit Kapila <amit.kapila16@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/CAA4eK1JOo0aJRhFHNWpj3hMwaTtNOopY34f1Lh_QD=z=+DrzWQ@mail.gmail.com
Backpatch-through: 19
The documentation previously said that pg_get_sequence_data() returns
a row of NULL values if the sequence does not exist or if the current
user lacks privileges on it. This was incomplete and could be misleading.
A nonexistent relation name is rejected during regclass input conversion,
while the function returns NULLs for a nonexistent relation OID and
several other cases.
This commit clarifies that the function returns NULLs when the specified
relation OID does not exist, the relation is not a sequence, the current
user lacks SELECT privilege on the sequence, the sequence belongs to
another session's temporary schema, or it is an unlogged sequence on
a standby server.
Author: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAA4eK1JOo0aJRhFHNWpj3hMwaTtNOopY34f1Lh_QD=z=+DrzWQ@mail.gmail.com
Backpatch-through: 19
When a string literal is provided as a property expression, the data
type of the property was set to "unknown", which may lead to various
failures when the property is used in GRAPH_TABLE or when its data
type is compared against other properties with the same name. To fix
this, call resolveTargetListUnknowns() on the targetlist of new
properties being added to resolve unknown type literals.
Reported-by: Noah Misch <noah@leadboat.com>
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/20260630173053.51.noahmisch%40microsoft.com
replace_property_refs() called expression_tree_mutator() with the root
of the expression tree as the input node. But
expression_tree_mutator() does not call the mutator function on the
root node, so the root node remains unchanged. If the root node is a
property reference or a lateral reference -- the two node kinds that
replace_property_refs_mutator() rewrites -- it is returned unchanged.
Modules after the rewriter do not know about property reference nodes,
resulting in "ERROR: unrecognized node type: 63". Since varlevelsup
of lateral references is not incremented, they are not resolved
correctly in the planner, leading to many different symptoms. Fix
this by calling replace_property_refs_mutator() directly from
replace_property_refs(), similar to how other mutator functions do.
The only case when a property reference or a lateral reference can be
the root of a GRAPH_TABLE expression tree is when it is a bare
property reference or a bare lateral reference in the WHERE clause.
The COLUMNS clause is passed to replace_property_refs() as a
targetlist. Every other expression has at least one expression node
covering the property reference or a lateral reference in the
expression tree. That explains why this bug was not seen so far.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://www.postgresql.org/message-id/flat/20260630173053.51.noahmisch%40microsoft.com
This change switches the implementation of wait and wakeups in the
module injection_points to not rely anymore on condition variables,
using a more primitive implementation based on atomics. The former
implementation required a PGPROC, making it impossible to inject waits
in the postmaster or during authentication. A couple of use cases have
popped up for these in the past, where this would have become handy.
The loop in the wait callback that relied on a condition variable is
replaced by an atomic counter, whose check increases over time in an
exponential manner (starts at 10us for quick responsiveness, up to
100ms).
This change may be backpatched at some point depending on how much
testing coverage is wanted. Let's limit ourselves to HEAD for now,
checking things first with the buildfarm.
Creating a wait still requires the SQL interface. We are looking at
expanding that with an alternative implementation, so as early startup
or authentication waits would become possible. This refactoring piece
is mandatory to achieve this goal.
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/aher0VsjJ8xeNgLq@paquier.xyz
This is a continuation of the work done in ac59a90bef. The
*GetDatum() macros for output should match with what the SQL functions
use as DatumGet*() in input.
Aleksander has spotted some of the areas patched here, for pageinspect.
I have spotted the rest while digging into the state of the tree.
There is no behavior change after this commit, since all the affected
values are small enough that the signed bit is never used.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/afLsqRjVqKK8hhKk@paquier.xyz
Commit aa606b9316 disallowed generated columns in COPY FROM WHERE
expressions, and commit 21c69dc73f disallowed system columns.
However, the COPY reference page still mentions only the restriction
on subqueries.
Update the documentation to also list generated columns and system
columns as unsupported in COPY FROM WHERE expressions.
Backpatch the generated-column documentation change to all supported
versions. Backpatch the system-column documentation change to v19,
where that restriction was introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEgxErc54yVOAVWCsr1O=8pgw4oKRPuEQ9mfhkoYGR_XA@mail.gmail.com
Backpatch-through: 14
Previously, when pg_recvlogical exited due to SIGINT or SIGTERM,
it could terminate without sending final feedback for the last decoded
changes it had already written locally. So, if pg_recvlogical was
restarted afterwards, the server-side logical replication slot could
still point behind those changes, causing them to be sent again.
Make pg_recvlogical send final feedback once more during SIGINT/SIGTERM
shutdown, before sending CopyDone. This gives the server one more chance
to advance the slot far enough to avoid resending already-written data,
so users are less likely to see duplicate decoded output after stopping
and restarting pg_recvlogical.
This remains a best-effort improvement rather than a guarantee. Depending
on when the signal arrives, pg_recvlogical can already have written
decoded output that the server cannot yet safely treat as confirmed, so a
later restart can still receive duplicate data.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwE83z9O=X7ADMsSa3e1EuP3_GgqHjFt5SmPDNxZo_wgJA@mail.gmail.com
When fixing up the targetlist and qpqual of an outer join, we must
account for the effects of the outer join. Vars and PHVs appearing
there are logically above the join, so they should have nullingrels
equal to the input Vars/PHVs' nullingrels plus the bit added by the
outer join.
Determining the effects of the outer join can be tricky when the join
has been commuted with another one per outer join identity 3. In this
case, the Vars/PHVs in the join's targetlist and qpqual should have
the same nullingrels that they would if the two joins had been done in
syntactic order. Unfortunately, in setrefs.c, we don't have enough
information to identify what that should be, so we have to use
superset nullingrels matches instead of exact ones.
However, we can tighten the check somewhat. Currently, we check
whether the jointype is JOIN_INNER and use NRM_SUPERSET if it is not.
We can improve this by checking whether the Join node has non-empty
ojrelids and using NRM_SUPERSET only in that case. This allows us to
perform exact matches in more situations.
To support this, we record the outer-join relids in Join plan nodes.
This information can also improve EXPLAIN (RANGE_TABLE) output by
showing which outer-join relids are completed by each Join plan node.
We may discover additional uses for this information in the future.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs482_DFHzQ079ZPp6c8UvmFdz3Jj+4K8tVRu9g2Bw34NPA@mail.gmail.com
With the changes in the previous commit, we can now use exact
nullingrels matches in all cases when fixing up expressions of
upper-level plan nodes that are not joins. Therefore, we can remove
the nrm_match parameter from fix_upper_expr(), along with the
corresponding field in fix_upper_expr_context.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs482_DFHzQ079ZPp6c8UvmFdz3Jj+4K8tVRu9g2Bw34NPA@mail.gmail.com
We have been using NRM_SUBSET to process NestLoopParams in setrefs.c,
because Vars or PHVs in NestLoopParam expressions may previously have
had nullingrels that were just subsets of those in the Vars or PHVs
actually available from the outer side.
Since 66e9df9f6, identify_current_nestloop_params ensures that any
Vars or PHVs seen in a NestLoopParam expression have nullingrels that
include exactly the outer-join relids that appear in the outer side's
output and can null the respective Var or PHV. As noted in that
commit's message, we can now safely use NRM_EQUAL to process
NestLoopParams in setrefs.c.
This patch makes that change and removes the definition of NRM_SUBSET,
along with all remaining checks for it, since it is no longer used.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs482_DFHzQ079ZPp6c8UvmFdz3Jj+4K8tVRu9g2Bw34NPA@mail.gmail.com
Necessary for 16-bit wchar_t platforms (Windows).
Other guards are just defensive. Also correct style issue with
branches.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260630012919.78@rfd.leadboat.com
Backpatch-through: 19
Previously, examine_simple_variable() would return early when a
subquery or CTE used GROUP BY or DISTINCT. It could detect uniqueness
for single-column cases, but for multi-column GROUP BY or DISTINCT,
selectivity estimation fell back on 1/DEFAULT_NUM_DISTINCT (1/200).
This produced wildly inaccurate estimates for filters and joins on
such columns, often leading the planner to choose nested loop joins
where hash joins would be far better. This was a significant factor
in poor TPC-DS benchmark performance.
For DISTINCT or GROUP BY key columns that are simple Vars, we now
recurse into the subquery to obtain the base table's stadistinct,
which remains valid after grouping (the set of distinct values is
preserved). However, MCV frequencies, histograms, and correlation
data are not valid because GROUP BY and DISTINCT change the frequency
distribution of key columns. So we strip all stats slots from the
copied stats tuple, causing callers like var_eq_const() to use the
1/ndistinct estimate instead. If stadistinct is stored as a negative
value (a fraction of the base table's row count), we convert it to an
absolute count so it is not misinterpreted relative to the subquery's
output row count.
stanullfrac is adjusted too, since grouping collapses NULLs. For a
single grouping key, at most one NULL group survives, so the null
fraction is 1/(ndistinct+1). For multiple grouping keys the null
fraction depends on the joint distribution of the keys, which we don't
have, so we approximate it as zero; NULLs collapse far more
aggressively than non-NULLs, so the real fraction is well below the
base table's, and erring low keeps estimates on the hash-join-favoring
side.
Non-key columns (e.g., aggregate outputs) continue to get no stats,
same as before.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: wenhui qiu <qiuwenhuifx@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49rWYrecgreDhKsfx3VSDW=qo35s+iAmgGu=wpARrM8_g@mail.gmail.com
Commit dcb0049523 accidentally changed the final expanded query's
condition to > 2 while rewriting the example into SQL operator notation.
The original query and the preceding rewritten forms all use >= 2,
and view expansion should preserve that qualification. This commit
changes the final condition from > 2 to >= 2.
Backpatch to all supported versions.
Reported-by: Yaroslav Saburov <y.saburov@gmail.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/178248467618.108999.9966122434342474006@wrigleys.postgresql.org
Backpatch-through: 14
If an expression containing an aggregate is evaluated above the plan
node that computes the aggregate, as happens with window functions or
with expressions postponed to above the final sort, setrefs.c replaces
the Aggref or WindowFunc with a Var referencing the lower node's
output. For SQL/JSON aggregates such as JSON_ARRAYAGG and
JSON_OBJECTAGG, deparsing the containing JsonConstructorExpr then
failed with "invalid JsonConstructorExpr underlying node type", since
get_json_agg_constructor() did not expect a Var there.
Fix by resolving the Var back to the underlying Aggref or WindowFunc
and deparsing the constructor as if the aggregate were computed at the
current node. The JsonConstructorExpr retains the RETURNING clause
and the ABSENT/NULL ON NULL and WITH UNIQUE options, and the arguments
come from the resolved aggregate, so the original JSON aggregate
syntax is reproduced in full. This mirrors how get_agg_expr() already
looks through such a Var when deparsing a combining aggregate.
Reported-by: Thom Brown <thom@linux.com>
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAA-aLv5QYTaMOk=Qhv6cgwceeHETZV8YJvWZ_rH+yVZCuchATA@mail.gmail.com
Backpatch-through: 16
heapam_tuple_complete_speculative() fetched a tuple from the slot only
to free it immediately afterwards, without ever using it.
The function only needs slot->tts_tid to complete or abort the
speculative insertion, so remove the unnecessary fetch and pfree().
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Japin Li <japinli@hotmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Discussion: https://www.postgresql.org/message-id/FCB61654-575D-4F08-AA7E-ED462EDE48A7@gmail.com
If the string is preceded only by Case Ignorable characters, don't
consider it to be a final sigma.
In the process, refactor so that the preceding and following
characters are found first, and then the rule is applied, to improve
clarity.
Discussion: https://postgr.es/m/c355354e6c3f4a7aafb047361b73db247260fca0.camel@j-davis.com
Backpatch-through: 18
GiST index killitems feature misbehaves for single-page GiST index,
i.e. one that has only a root page. This is caused by the GiST scan's
curBlkno variable not being initialized for the first-to-scan page,
which is the root page. Fix this by moving the initializing of
curBlkno into gistScanPage(), where we also set the related curPageLSN
variable.
Commit 377b7ab145 actually added a regression test for this already,
but it merely noted that it's not working and memorized the result
where the items were not killed. Now they are, as the test shows.
This has been broken all along, but since it's just a very minor
performance issue on tiny tables, I didn't bother backpatching it.
Author: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Soumya S Murali <soumyamurali.work@gmail.com>
Discussion: https://postgr.es/m/CALdSSPgZWX_D8%2BFx4YQqRN5eW5iSx_rJdqQhCfdWTvqKXVfJ4w%40mail.gmail.com
Discussion: https://postgr.es/m/lxzj26ga6ippdeunz6kuncectr5gfuugmm2ry22qu6hcx6oid6@lzx3sjsqhmt6
Only "tombstone" files (first segment of main fork) are unlinked after
checkpoints, so rename the function and remove the extra arguments to
make that more clear.
Additionally, add an assertion in mdunlinkfiletag() that the FileTag
only contains expected values.
Author: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAEze2WjfP95SL_Hsu7GzYXLnQyEsT49zOnNvbY_mBLCFiQra1g@mail.gmail.com
Adding a GRANT caused pg_dump to emit a useless REVOKE + GRANT of owner
privileges, as seen in a dump of the regression database:
REVOKE ALL ON PROPERTY GRAPH graph_rls_schema.cabinet FROM nm;
GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO nm;
GRANT ALL ON PROPERTY GRAPH graph_rls_schema.cabinet TO PUBLIC;
For normal dumps, this has no functional consequences. For --no-owner
restores, the extra statements may fail or locate unrelated users of the
destination cluster.
The problem was pg_dump assuming NULL relacl implies acldefault('r'),
the default for TABLE. Fix by teaching acldefault() to retrieve the
PROPERTY GRAPH default ACL. So pg_dump can still dump from 19beta1, use
acldefault('g') for v20+ only. For v19, use a hard-coded snapshot of
the v19 default.
information_schema.pg_property_graph_privileges also misused
acldefault('r'), but its "c.prtype IN ('SELECT')" predicate compensated
for it. Switch to the new acldefault('g') for clarity. Bump catversion
since a new view won't work with old binaries. Back-patch to v19, which
introduced PROPERTY GRAPH.
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/20260630023308.c7.noahmisch@microsoft.com
Backpatch-through: 19
This commit cleans up volatile qualifiers that fit the below
criteria:
* Accesses to shared memory protected by a spinlock or LWLock.
Before commit 0709b7ee72, callers had to use volatile when
accessing spinlock-protected shared memory. Since spinlock
acquire/release became compiler barriers, and because LWLocks
provide the same guarantee, that is no longer necessary. These
either predate that change or were cargo-culted from code that did.
* Pointers used only to find the address of a member. The volatile
qualifier only affects accesses made by dereferencing the pointer,
so it is unnecessary there.
* Accesses to struct members that are marked volatile in the struct
definition. There's no need to mark these pointers volatile,
either.
* Leftovers from removed PG_TRY blocks. These were marked volatile
to protect a value that is modified inside a PG_TRY block, but the
PG_TRY has since been removed.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://postgr.es/m/akQ5eJR1tCCXme8e%40nathan
The previous commit strengthened a workaround for a hang when large
messages are split across TLS records/GSS tokens. Because that
workaround is implemented in libpq internals, it can only help us when
libpq itself is polling on the socket. In nonblocking situations,
where the client above libpq is expected to poll, the same bugs can
show up.
As a contrived example, consider a large protocol-2.0 error coming
back from a server during PQconnectPoll(), split in an odd way across
two records:
-- TLS record (8192-byte payload) --
EEEE[...repeated a total of 8192 times]
-- TLS record (8193-byte payload) --
EEEE[...repeated a total of 8192 times]\0
The first record will fill the first half of the libpq receive buffer,
which is 16k long by default. The second record completely fills the
last half with its first 8192 bytes, leaving the terminating NULL in
the OpenSSL buffer. Since we still haven't seen the terminator at our
level, PQconnectPoll() will return PGRES_POLLING_READING, expecting to
come back when the server has sent "the rest" of the data. But there
is nothing left to read from the socket; OpenSSL had to pull all of
the data in the 8193-byte record off of the wire to decrypt it.
A real server would probably not split up the records this way, nor
keep the connection open after sending a fatal connection error. But
servers that regularly use larger TLS records can get the libpq
receive buffer into the same state if DataRows are big enough, as
reported on the list. While the PostgreSQL server doesn't use larger
TLS records like that, other non-PostgreSQL servers that implement the
wire protocol are known to do that, as well as proxies that sit
between the server and the client
This is a layering violation. libpq makes decisions based on data in
the application buffer, above the transport buffer (whether SSL or
GSS), but clients are polling the socket below the transport buffer.
One way to fix this in a backportable way, without changing APIs too
much, is to ensure data never stays in the transport buffer. Then
pqReadData's postconditions will look similar for both raw sockets and
SSL/GSS: any available data is either in the application buffer, or
still on the socket.
Building on the prior commit, make pqReadData() to drain all pending
data from the transport layer into conn->inBuffer, expanding the
buffer as necessary. This is not particularly efficient from an
architectural perspective (the pqsecure_read() implementations take
care to fit their packets into the current buffer, and that effort is
now completely discarded), but it's hopefully easier to reason about
than a full rewrite would be for the back branches.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Reviewed-by: Mark Dilger <mark.dilger@enterprisedb.com>
Reviewed-by: solai v <solai.cdac@gmail.com>
Reported-by: Lars Kanis <lars@greiz-reinsdorf.de>
Discussion: https://postgr.es/m/2039ac58-d3e0-434b-ac1a-2a987f3b4cb1%40greiz-reinsdorf.de
Backpatch-through: 14
An extra check for pending bytes in the SSL layer has been part of
pqReadReady() for a very long time (79ff2e96d). But when GSS transport
encryption was added, it didn't receive the same treatment. (As
79ff2e96d notes, "The bug that I fixed in this patch is exceptionally
hard to reproduce reliably.")
Without that check, it's possible to hit a hang in gssencmode, if the
server splits a large libpq message such that the final message in a
streamed response is part of the same wrapped token as the split
message:
DataRowDataRowDataRowDataRowDataRowData
-- token boundary --
RowDataRowCommandCompleteReadyForQuery
If the split message takes up enough memory to nearly fill libpq's
receive buffer, libpq may return from pqReadData() before the later
messages are pulled out of the PqGSSRecvBuffer. Without additional
socket activity from the server, pqReadReady() (via pqSocketCheck())
will never again return true, hanging the connection.
Pull the pending-bytes check into the pqsecure API layer, where both
SSL and GSS now implement it.
Note that this does not fix the root problem! Third party clients of
libpq have no way to call pqsecure_read_is_pending() in their own
polling. This just brings the GSS implementation up to par with the
existing SSL workaround; a broader fix is left to a subsequent commit.
In preparation for the broader fix, this patch already changes the
*_read_pending() functions to return the number of bytes in the buffer
rather than just a boolean. The current callers don't need that, but
the subsequent fix will.
Author: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/CAOYmi%2BmpymrgZ76Jre2dx_PwRniS9YZojwH0rZnTuiGHCsj0rA%40mail.gmail.com
Backpatch-through: 14
Three error messages were using the default file name pg_hosts.conf
and not the variable backing the GUC, which would make logging be
confusing for users who have renamed the file using the GUC. Fix
by consistently using the HostsFileName variable.
Backpatch down to v19 where serverside SNI was introduced.
Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Surya Poondla <suryapoondla4@gmail.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAN4CZFMARYjQfgyRaCKOXDO=Q91kuKn=pSC02DAOOr23ojhEGQ@mail.gmail.com
Backpatch-through: 19
ParallelBackupStart() stored _beginthreadex()'s return value as the
worker's thread handle without checking it. On failure that value is 0,
which would later reach WaitForMultipleObjects() as a null handle, caught
only by an Assert. The fork() path already calls pg_fatal() when it
fails; do the same for _beginthreadex(), as pgbench does.
Author: Bryan Green <dbryan.green@gmail.com>
Discussion: https://www.postgresql.org/message-id/8c712d76-ecf7-4749-a6d8-dddc01f298ec@gmail.com
Backpatch-through: 14
9d9c02ccd added code to allow the executor to stop early when processing
WindowAgg nodes where a monotonic window function starts producing
values that result in a pushed-down qual no longer matching, and will
never match again due to the window function's monotonic properties.
That commit requires a SupportRequestWFuncMonotonic to exist on the
window function and for it to detect when the function is monotonic. For
COUNT(ANY) and COUNT(*), the support function failed to consider some
cases where the WindowClause used EXCLUDE to exclude certain rows from
being aggregated. Some WindowClause definitions mean we aggregate rows
that come after the current row, and when processing those rows later,
if we EXCLUDE certain rows, the monotonic property can be broken.
Wrongly treating the COUNT(*) or COUNT(ANY) aggregate as monotonic could
lead to rows being filtered that should not be filtered from the result
set.
Another issue was that the support function for the COUNT aggregate
mistakenly thought that a WindowClause without an ORDER BY meant that
the results would be both monotonically increasing and decreasing, but
that's only true when in RANGE mode, where all rows are peers.
It is possible to support various cases that do have an EXCLUDE clause,
but getting the logic correct for the exact set of cases that are valid
is quite complex and would likely better be left for a future project.
Here, we mostly disable run condition pushdown when there is an EXCLUDE
clause unless the clause is for EXCLUDE CURRENT ROW, uses COUNT(*)
(rather than COUNT(ANY)), and the window aggregate has no FILTER clause.
Bug: #19533
Reported-by: Qifan Liu <imchifan@163.com>
Author: Chengpeng Yan <chengpeng_yan@outlook.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: John Naylor <johncnaylorls@gmail.com>
Discussion: https://postgr.es/m/19533-413a1014e5d0e766@postgresql.org
Backpatch-through: 15
This was the dominant style already, but some places used %llu
instead. Since off_t/pgoff_t are signed types, using %lld seems a
better match, and it might handle obscure error conditions with
negative values better.
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/20ce62fa-47fc-457b-b504-12f3c1651726%40eisentraut.org
