This is similar to 9004abf620, but this time for the write part of the
stats file. The code is changed so as, rather than referring to
individual members of PgStat_Snapshot in an order based on their
PgStat_Kind value, a loop based on pgstat_kind_infos is used to retrieve
the contents to write from the snapshot structure, for a size of
PgStat_KindInfo's shared_data_len.
This requires the addition to PgStat_KindInfo of an offset to track the
location of each fixed-numbered stats in PgStat_Snapshot. This change
is useful to make this area of the code more easily pluggable, and
reduces the knowledge of specific fixed-numbered kinds in pgstat.c.
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Zot5bxoPYdS7yaoy@paquier.xyz
xmlParseInNodeContext has basically the same functionality with
a different API: we have to supply an xmlNode that's attached to a
document rather than just the document. That's not hard though.
The benefits are two:
* Early 2.13.x releases of libxml2 contain a bug that causes
xmlParseBalancedChunkMemory to return the wrong status value in some
cases. This breaks our regression tests. While that bug is now fixed
upstream and will probably never be seen in any production-oriented
distro, it is currently a problem on some more-bleeding-edge-friendly
platforms.
* xmlParseBalancedChunkMemory is considered to depend on libxml2's
semi-deprecated SAX1 APIs, and will go away when and if they do.
There may already be libxml2 builds out there that lack this function.
So there are both short- and long-term reasons to make this change.
While here, avoid allocating an xmlParserCtxt in DOCUMENT parse mode,
since that code path is not going to use it.
Like 066e8ac6e, this will need to be back-patched. This is just a
trial commit to see if the buildfarm agrees that we can use
xmlParseInNodeContext unconditionally.
Erik Wienhold and Tom Lane, per report from Frank Streitzig.
Discussion: https://postgr.es/m/trinity-b0161630-d230-4598-9ebc-7a23acdb37cb-1720186432160@3c-app-gmx-bap25
Discussion: https://postgr.es/m/trinity-361ba18b-541a-4fe7-bc63-655ae3a7d599-1720259822452@3c-app-gmx-bs01
The numeric round() and trunc() functions clamp the scale argument to
the range between +/- NUMERIC_MAX_RESULT_SCALE (2000), which is much
smaller than the actual allowed range of type numeric. As a result,
they return incorrect results when asked to round/truncate more than
2000 digits before or after the decimal point.
Fix by using the correct upper and lower scale limits based on the
actual allowed (and documented) range of type numeric.
While at it, use the new NUMERIC_WEIGHT_MAX constant instead of
SHRT_MAX in all other overflow checks, and fix a comment thinko in
power_var() introduced by e54a758d24 -- the minimum value of
ln_dweight is -NUMERIC_DSCALE_MAX (-16383), not -SHRT_MAX, though this
doesn't affect the point being made in the comment, that the resulting
local_rscale value may exceed NUMERIC_MAX_DISPLAY_SCALE (1000).
Back-patch to all supported branches.
Dean Rasheed, reviewed by Joel Jacobson.
Discussion: https://postgr.es/m/CAEZATCXB%2BrDTuMjhK5ZxcouufigSc-X4tGJCBTMpZ3n%3DxxQuhg%40mail.gmail.com
590b045c3 made it so tuplestore.c would store tuples inside a
generation.c memory context. After fixing a bug report in 97651b013, it
seems that it's probably best not to allocate BufFile related
allocations in that context. Let's keep it just for tuple data.
This adjusts the code to switch to the Tuplestorestate.context's parent,
which is the MemoryContext that tuplestore_begin_common() was called in.
It does not seem worth adding a new field in Tuplestorestate to store
this when we can access it by looking at the Tuplestorestate's
context's parent.
Discussion: https://postgr.es/m/CAApHDvqFt_CdJtSr+E9YLZb7jZAyRCy3hjQ+ktM+dcOFVq-xkg@mail.gmail.com
This only affects MEMORY_CONTEXT_CHECKING builds.
This fixes an off-by-one issue in GenerationRealloc() where the
fast-path code which tries to reuse the existing allocation if the
existing chunk is >= the new requested size. The code there thought it
was always ok to use the existing chunk, but when oldsize == size there
isn't enough space to store the sentinel byte. If both sizes matched
exactly set_sentinel() would overwrite the first byte beyond the chunk
and then subsequent GenerationRealloc() calls could then fail the
Assert(chunk->requested_size < oldsize) check which is trying to ensure
the chunk is large enough to store the sentinel.
The same issue does not exist in aset.c as the sentinel checking code
only adds a sentinel byte if there's enough space in the chunk.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/49275921-7b39-41af-5eb8-97b50ce3312e@gmail.com
Backpatch-through: 16, where the problem was introduced by 0e480385e
Each of max_connections, max_worker_processes,
autovacuum_max_workers, and max_wal_senders has a GUC check hook
that verifies the sum of those GUCs does not exceed a hard-coded
limit (see the comment for MAX_BACKENDS in postmaster.h). In
general, the hooks effectively guard against egregious
misconfigurations.
However, this approach has some problems. Since these check hooks
are called as each GUC is assigned its user-specified value, only
one of the hooks will be called with all the relevant GUCs set. If
one or more of the user-specified values are less than the initial
values of the GUCs' underlying variables, false positives can
occur.
Furthermore, the error message emitted when one of the check hooks
fails is not tremendously helpful. For example, the command
$ pg_ctl -D . start -o "-c max_connections=262100 -c max_wal_senders=10000"
fails with the following error:
FATAL: invalid value for parameter "max_wal_senders": 10000
Fortunately, there is an extra copy of this check in
InitializeMaxBackends() that we can rely on, so this commit removes
the aforementioned GUC check hooks in favor of that one. It also
enhances the error message to clearly show the values of the
relevant GUCs and the hard-coded limit their sum may not exceed.
The downside of this change is that server startup progresses
further before failing due to such misconfigurations (thus taking
longer), but these failures are expected to be rare, so we don't
anticipate any real harm in practice.
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/ZnMr2k-Nk5vj7T7H%40nathan
This can be used to load an injection point and prewarm the
backend-level cache before running it, to avoid issues if the point
cannot be loaded due to restrictions in the code path where it would be
run, like a critical section where no memory allocation can happen
(load_external_function() can do allocations when expanding a library
name).
Tests can use a macro called INJECTION_POINT_LOAD() to load an injection
point. The test module injection_points gains some tests, and a SQL
function able to load an injection point.
Based on a request from Andrey Borodin, who has implemented a test for
multixacts requiring this facility.
Reviewed-by: Andrey Borodin
Discussion: https://postgr.es/m/ZkrBE1e2q2wGvsoN@paquier.xyz
Here we make tuplestore.c use a generation.c memory context rather than
allocating tuples into the CurrentMemoryContext, which primarily is the
ExecutorState or PortalHoldContext memory context. Not having a
dedicated context can cause the CurrentMemoryContext context to become
bloated when pfree'd chunks are not reused by future tuples. Using
generation speeds up users of tuplestore.c, such as the Materialize,
WindowAgg and CTE Scan executor nodes. The main reason for the speedup is
due to generation.c being more memory efficient than aset.c memory
contexts. Specifically, generation does not round sizes up to the next
power of 2 value. This both saves memory, allowing more tuples to fit in
work_mem, but also makes the memory usage more compact and fit on fewer
cachelines. One benchmark showed up to a 22% performance increase in a
query containing a Materialize node. Much higher gains are possible if
the memory reduction prevents tuplestore.c from spilling to disk. This is
especially true for WindowAgg nodes where improvements of several thousand
times are possible if the memory reductions made here prevent tuplestore
from spilling to disk.
Additionally, a generation.c memory context is much better suited for this
job as it works well with FIFO palloc/pfree patterns, which is exactly how
tuplestore.c uses it. Because of the way generation.c allocates memory,
tuples consecutively stored in tuplestores are much more likely to be
stored consecutively in memory. This allows the CPU's hardware prefetcher
to work more efficiently as it provides a more predictable pattern to
allow cachelines for the next tuple to be loaded from RAM in advance of
them being needed by the executor.
Using a dedicated memory context for storing tuples also allows us to more
efficiently clean up the memory used by the tuplestore as we can reset or
delete the context rather than looping over all stored tuples and
pfree'ing them one by one.
Also, remove a badly placed USEMEM call in readtup_heap(). The tuple
wasn't being allocated in the Tuplestorestate's context, so no need to
adjust the memory consumed by the tuplestore there.
Author: David Rowley
Reviewed-by: Matthias van de Meent, Dmitry Dolgov
Discussion: https://postgr.es/m/CAApHDvp5Py9g4Rjq7_inL3-MCK1Co2CRt_YWFwTU2zfQix0p4A@mail.gmail.com
Up until now, there was no ability to easily determine if a Material
node caused the underlying tuplestore to spill to disk or even see how
much memory the tuplestore used if it didn't.
Here we add some new functions to tuplestore.c to query this information
and add some additional output in EXPLAIN ANALYZE to display this
information for the Material node.
There are a few other executor node types that use tuplestores, so we
could also consider adding these details to the EXPLAIN ANALYZE for
those nodes too. Let's consider those independently from this. Having
the tuplestore.c infrastructure in to allow that is step 1.
Author: David Rowley
Reviewed-by: Matthias van de Meent, Dmitry Dolgov
Discussion: https://postgr.es/m/CAApHDvp5Py9g4Rjq7_inL3-MCK1Co2CRt_YWFwTU2zfQix0p4A@mail.gmail.com
All the errors triggered in the code paths patched here would cause the
backend to issue an internal_error errcode, which is a state that should
be used only for "can't happen" situations. However, these code paths
are reachable by the regression tests, and could be seen by users in
valid cases. Some regression tests expect internal errcodes as they
manipulate the backend state to cause corruption (like checksums), or
use elog() because it is more convenient (like injection points), these
have no need to change.
This reduces the number of internal failures triggered in a check-world
by more than half, while providing correct errcodes for these valid
cases.
Reviewed-by: Robert Haas
Discussion: https://postgr.es/m/Zic_GNgos5sMxKoa@paquier.xyz
This changes pgstat.c so as the three types of entries that can exist in
a pgstats file are not hardcoded anymore, replacing them with
descriptively-named macros, when reading and writing stats files:
- 'N' for named entries, like replication slot stats.
- 'S' for entries identified by a hash.
- 'E' for the end-of-file
This has come up while working on making this area of the code more
pluggable. The format of the stats file is unchanged, hence there is no
need to bump PGSTAT_FILE_FORMAT_ID.
Reviewed-by: Bertrand Drouvot
Discussion: https://postgr.es/m/Zmqm9j5EO0I4W8dx@paquier.xyz
We have in launch_backend.c:
/*
* The following need to be available to the save/restore_backend_variables
* functions. They are marked NON_EXEC_STATIC in their home modules.
*/
extern slock_t *ShmemLock;
extern slock_t *ProcStructLock;
extern PGPROC *AuxiliaryProcs;
extern PMSignalData *PMSignalState;
extern pg_time_t first_syslogger_file_time;
extern struct bkend *ShmemBackendArray;
extern bool redirection_done;
That comment is not completely true: ShmemLock, ShmemBackendArray, and
redirection_done are not in fact NON_EXEC_STATIC. ShmemLock once was,
but was then needed elsewhere. ShmemBackendArray was static inside
postmaster.c before launch_backend.c was created. redirection_done
was never static.
This patch moves the declaration of ShmemLock and redirection_done to
a header file.
ShmemBackendArray gets a NON_EXEC_STATIC. This doesn't make a
difference, since it only exists if EXEC_BACKEND anyway, but it makes
it consistent.
After that, the comment is now correct.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
src/backend/libpq/pqcomm.c: "postmaster/postmaster.h" for Unix_socket_group, Unix_socket_permissions
src/backend/utils/init/globals.c: "postmaster/postmaster.h" for MyClientSocket
src/backend/utils/misc/guc_tables.c: "utils/rls.h" for row_security
src/backend/utils/sort/tuplesort.c: "utils/guc.h" for trace_sort
Nothing currently diagnoses missing includes for global variables, but
this is being cleaned up, and these ones had an obvious header file
available.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://www.postgresql.org/message-id/flat/e0a62134-83da-4ba4-8cdb-ceb0111c95ce@eisentraut.org
This commit removes unused variables and routines from some perl code
that have accumulated across the years. This touches the following
areas:
- Wait event generation script.
- AdjustUpgrade.pm.
- TAP perl code
Author: Alexander Lakhin
Reviewed-by: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/70b340bc-244a-589d-ef8b-d8aebb707a84@gmail.com
Up to now, committing a transaction has caused CurrentMemoryContext to
get set to TopMemoryContext. Most callers did not pay any particular
heed to this, which is problematic because TopMemoryContext is a
long-lived context that never gets reset. If the caller assumes it
can leak memory because it's running in a limited-lifespan context,
that behavior translates into a session-lifespan memory leak.
The first-reported instance of this involved ProcessIncomingNotify,
which is called from the main processing loop that normally runs in
MessageContext. That outer-loop code assumes that whatever it
allocates will be cleaned up when we're done processing the current
client message --- but if we service a notify interrupt, then whatever
gets allocated before the next switch to MessageContext will be
permanently leaked in TopMemoryContext. sinval catchup interrupts
have a similar problem, and I strongly suspect that some places in
logical replication do too.
To fix this in a generic way, let's redefine the behavior as
"CommitTransactionCommand restores the memory context that was current
at entry to StartTransactionCommand". This clearly fixes the issue
for the notify and sinval cases, and it seems to match the mental
model that's in use in the logical replication code, to the extent
that anybody thought about it there at all.
For consistency, likewise make subtransaction exit restore the context
that was current at subtransaction start (rather than always selecting
the CurTransactionContext of the parent transaction level). This case
has less risk of resulting in a permanent leak than the outer-level
behavior has, but it would not meet the principle of least surprise
for some CommitTransactionCommand calls to restore the previous
context while others don't.
While we're here, also change xact.c so that we reset
TopTransactionContext at transaction exit and then re-use it in later
transactions, rather than dropping and recreating it in each cycle.
This probably doesn't save a lot given the context recycling mechanism
in aset.c, but it should save a little bit. Per suggestion from David
Rowley. (Parenthetically, the text in src/backend/utils/mmgr/README
implies that this is how I'd planned to implement it as far back as
commit 1aebc3618 --- but the code actually added in that commit just
drops and recreates it each time.)
This commit also cleans up a few places outside xact.c that were
needlessly making TopMemoryContext current, and thus risking more
leaks of the same kind. I don't think any of them represent
significant leak risks today, but let's deal with them while the
issue is top-of-mind.
Per bug #18512 from wizardbrony. Commit to HEAD only, as this change
seems to have some risk of breaking things for some callers. We'll
apply a narrower fix for the known-broken cases in the back branches.
Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us
The standby_slot_names GUC allows the specification of physical standby
slots that must be synchronized before the logical walsenders associated
with logical failover slots. However, for this purpose, the GUC name is
too generic.
Author: Hou Zhijie
Reviewed-by: Bertrand Drouvot, Masahiko Sawada
Backpatch-through: 17
Discussion: https://postgr.es/m/ZnWeUgdHong93fQN@momjian.us
Shared statistics with a fixed number of objects are read from the stats
file in pgstat_read_statsfile() using members of PgStat_ShmemControl and
following an order based on their PgStat_Kind value.
Instead of being explicit, this commit changes the stats read to iterate
over the pgstat_kind_infos array to find the memory locations to read
into, based on a new shared_ctl_off in PgStat_KindInfo that can be used
to define the position of this stats kind in shared memory. This makes
the read logic simpler, and eases the introduction of future
improvements aimed at making this area more pluggable for external
modules.
Original idea suggested by Andres Freund.
Author: Tristan Partin
Reviewed-by: Andres Freund, Michael Paquier
Discussion: https://postgr.es/m/D12SQ7OYCD85.20BUVF3DWU5K7@neon.tech
This field is used to track if a stats kind can use a custom format
representation on disk when reading or writing its stats case. On HEAD,
this exists for replication slots stats, that need a mapping between an
internal index ID and the slot names.
named_on_disk is currently used nowhere and the callbacks
to_serialized_name and from_serialized_name are in charge of checking if
the serialization of the stats data should apply, so let's remove it.
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/ZmKVlSX_T5YvIOsd@paquier.xyz
Instead of looking up casts at parse time for converting the result
of JsonPath* query functions to the specified or the default
RETURNING type, always perform the conversion at runtime using either
the target type's input function or the function
json_populate_type().
There are two motivations for this change:
1. json_populate_type() coerces to types with typmod such that any
string values that exceed length limit cause an error instead of
silent truncation, which is necessary to be standard-conforming.
2. It was possible to end up with a cast expression that doesn't
support soft handling of errors causing bugs in the of handling
ON ERROR clause.
JsonExpr.coercion_expr which would store the cast expression is no
longer necessary, so remove.
Bump catversion because stored rules change because of the above
removal.
Reported-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Jian He <jian.universality@gmail.com>
Discussion: Discussion: https://postgr.es/m/202405271326.5a5rprki64aw%40alvherre.pgsql
As coded, an injection point could be loaded into the local cache
without the LWLock InjectionPointLock taken, hence a point detached and
re-attached concurrently of a point running calling InjectionPointRun()
may finish by loading a callback it did no set initially. Based on all
the cases discussed until now on the lists, it is fine to delay the lock
release until the callback is run, so let's do that.
While on it, remove a useless LWLockRelease() called before an error in
InjectionPointAttach().
Per discussion with Heikki Linnakangas and Noah Misch.
Discussion: https://postgr.es/m/e1ffb822-054e-4006-ac06-50532767f75b@iki.fi
This extends ad98fb1422 to invals of
inplace updates. Trouble requires an inplace update of a catalog having
a TOAST table, so only pg_database was at risk. (The other catalog on
which core code performs inplace updates, pg_class, has no TOAST table.)
Trouble would require something like the inplace-inval.spec test.
Consider GRANT ... ON DATABASE fetching a stale row from cache and
discarding a datfrozenxid update that vac_truncate_clog() has already
relied upon. Back-patch to v12 (all supported versions).
Reviewed (in an earlier version) by Robert Haas.
Discussion: https://postgr.es/m/20240114201411.d0@rfd.leadboat.com
Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com
Make the isolation harness recognize injection_points wait events as a
type of blocked state. Test an extant inplace-update bug.
Reviewed by Robert Haas and Michael Paquier.
Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com
errfinish() assumes that the __FUNC__ and __FILE__ arguments it's
passed are compile-time constant strings that can just be pointed
to rather than physically copied. However, it's possible for LLVM
to generate code in which those pointers point into a dynamically
loaded code segment. If that segment gets unloaded before we're
done with the ErrorData struct, we have dangling pointers that
will lead to SIGSEGV. In simple cases that won't happen, because we
won't unload LLVM code before end of transaction. But it's possible
to happen if the error is thrown within end-of-transaction code run by
_SPI_commit or _SPI_rollback, because since commit 2e517818f those
functions clean up by ending the transaction and starting a new one.
Rather than fixing this by adding pstrdup() overhead to every
elog/ereport sequence, let's fix it by copying the risky pointers
in CopyErrorData(). That solves it for _SPI_commit/_SPI_rollback
because they use that function to preserve the error data across
the transaction end/restart sequence; and it seems likely that
any other code doing something similar would need to do that too.
I'm suspicious that this behavior amounts to an LLVM bug (or a
bug in our use of it?), because it implies that string constant
references that should be pointer-equal according to a naive
understanding of C semantics will sometimes not be equal.
However, even if it is a bug and someday gets fixed, we'll have
to cope with the current behavior for a long time to come.
Report and patch by me. Back-patch to all supported branches.
Discussion: https://postgr.es/m/1565654.1719425368@sss.pgh.pa.us
When pgstats is initialized for a backend, it uses dsa_attach_in_place()
without a "segment" provided. Hence, no callback is registered to
automatically release the DSA attached once a backend exits. Not doing
any cleanup causes the reference count of the pgstats DSA to
continuously increment, at some point overflowing it (the more the
number of connections, the faster it is to reach this state). Once the
reference count overflows and then gets back to 0, new backends are not
able to attach to the pgstats DSA, failing startup.
This issue is resolved by adding in the pgstats shutdown hook a call to
dsa_release_in_place(), ensuring that the DSA attached at backend
startup is correctly released, keeping the reference count at bay.
The author of this patch has been able to see this issue on a server
with a long uptime and a high connection turnover.
Issue introduced by 5891c7a8ed, so backpatch down to 15.
Author: Anthonin Bonnefoy
Discussion: https://postgr.es/m/CAO6_XqqJbJBL=M7Ym13TcB4Xnq58vRa2jcC+gwEPBgbAda6B1Q@mail.gmail.com
Backpatch-through: 15
Previously, GetJsonPathVar() allowed a jsonpath expression to
reference any prefix of a PASSING variable's name. For example, the
following query would incorrectly work:
SELECT JSON_QUERY(context_item, jsonpath '$xy' PASSING val AS xyz);
The fix ensures that the length of the variable name mentioned in a
jsonpath expression matches exactly with the name of the PASSING
variable before comparing the strings using strncmp().
Reported-by: Alvaro Herrera (off-list)
Discussion: https://postgr.es/m/CA+HiwqFGkLWMvELBH6E4SQ45qUHthgcRH6gCJL20OsYDRtFx_w@mail.gmail.com
Commit 534287403 invented SHARED_DEPENDENCY_INITACL entries in
pg_shdepend, but installed them only for non-owner roles mentioned
in a pg_init_privs entry. This turns out to be the wrong thing,
because there is nothing to cue REASSIGN OWNED to go and update
pg_init_privs entries when the object's ownership is reassigned.
That leads to leaving dangling entries in pg_init_privs, as
reported by Hannu Krosing. Instead, install INITACL entries for
all roles mentioned in pg_init_privs entries (except pinned roles),
and change ALTER OWNER to not touch them, just as it doesn't
touch pg_init_privs entries.
REASSIGN OWNED will now substitute the new owner OID for the old
in pg_init_privs entries. This feels like perhaps not quite the
right thing, since pg_init_privs ought to be a historical record
of the state of affairs just after CREATE EXTENSION. However,
it's hard to see what else to do, if we don't want to disallow
dropping the object's original owner. In any case this is
better than the previous do-nothing behavior, and we're unlikely
to come up with a superior solution in time for v17.
While here, tighten up some coding rules about how ACLs in
pg_init_privs should never be null or empty. There's not any
obvious reason to allow that, and perhaps asserting that it's
not so will catch some bugs. (We were previously inconsistent
on the point, with some code paths taking care not to store
empty ACLs and others not.)
This leaves recordExtensionInitPrivWorker not doing anything
with its ownerId argument, but we'll deal with that separately.
catversion bump forced because of change of expected contents
of pg_shdepend when pg_init_privs entries exist.
Discussion: https://postgr.es/m/CAMT0RQSVgv48G5GArUvOVhottWqZLrvC5wBzBa4HrUdXe9VRXw@mail.gmail.com
The manual says clearly that punctuation in the input of
websearch_to_tsquery() is ignored, except for the special cases
of dashes and quotes. However, this failed for cases like
"(foo bar) or something", or in general an ISOPERATOR character
in front of the "or". We'd switch back to WAITOPERAND state,
then ignore the operator character while remaining in that state,
and then reach the "or" in WAITOPERAND state which (intentionally)
makes us treat it as data.
The fix is simple enough: if we see an ISOPERATOR character while in
WAITOPERATOR state, we have to skip it while staying in that state.
(We don't need to worry about other punctuation characters: those will
be consumed as though they were words, but then rejected by lexizing.)
In v14 and up (since commit eb086056f) we can simplify the code a bit
more too, because there is no longer a reason for the WAITOPERAND
state to distinguish between quoted and unquoted operands.
Per bug #18479 from Manos Emmanouilidis. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/18479-d9b46e2fc242c33e@postgresql.org
Make sure that function declarations use names that exactly match the
corresponding names from function definitions in a few places. These
inconsistencies were all introduced during Postgres 17 development.
pg_bsd_indent still has a couple of similar inconsistencies, which I
(pgeoghegan) have left untouched for now.
This commit was written with help from clang-tidy, by mechanically
applying the same rules as similar clean-up commits (the earliest such
commit was commit 035ce1fe).
Floris Van Nee has reported a bug in the pgstats facility where a stats
entry already dropped would get again dropped. This case should not
happen, still the error generated did not offer any details about the
stats entry getting dropped.
This commit improves the error message generated to inform about the
stats entry kind, database OID, object OID and refcount, which should
help to debug more the problem reported. Bertrand Drouvot has been
independently able to reach this error path while writing a new feature,
and more details about the failure would have been helpful for
debugging.
Author: Andres Freund, Bertrand Drouvot
Discussion: https://postgr.es/m/20240505160915.6boysum4f34siqct@awork3.anarazel.de
Discussion: https://postgr.es/m/ZkM30paAD8Cr/Bix@ip-10-97-1-34.eu-west-3.compute.internal
Backpatch-through: 15
ReorderBufferImmediateInvalidation() executes invalidation messages in
an aborted transaction. However, RelationFlushRelation sometimes
required a valid resource owner, to temporarily increment the refcount
of the relache entry. Commit b8bff07daa worked around that in the main
subtransaction abort function, AbortSubTransaction(), but missed this
similar case in ReorderBufferImmediateInvalidation().
To fix, introduce a separate function to invalidate a relcache
entry. It does the same thing as RelationClearRelation(rebuild==true)
does when outside a transaction, but can be called without
incrementing the refcount.
Add regression test. Before this fix, it failed with:
ERROR: ResourceOwnerEnlarge called after release started
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce9772721c@gmail.com
Concurrent activity around replication slot creation and drop could
cause a replication slot to use a stats entry it should not have used
when created, triggering an assertion failure when retrieving an
inconsistent entry from the dshash table used by the stats facility.
The issue is that pgstat_drop_replslot() calls pgstat_drop_entry()
without checking the result. If pgstat_drop_entry() cannot free the
entry related to the object dropped, pgstat_request_entry_refs_gc()
should be called. AtEOXact_PgStat_DroppedStats() and surrounding
routines dropping stats entries already do that.
This is documented in pgstat_internal.h, but let's add a comment at the
top of pgstat_drop_entry() as that can be easy to miss.
Reported-by: Alexander Lakhin
Author: Floris Van Nee
Analyzed-by: Andres Freund
Discussion: https://postgr.es/m/17947-b9554521ad963c9c@postgresql.org
Backpatch-through: 15
They were under "File Locations", which doesn't make sense. Move them
to Resource Usage / Memory, which matches their categorization in the
source code and in the documentation.
Commit 3e1a373e2 missed teaching DecodeTimeOnly the same "ptype"
manipulations it added to DecodeDateTime. While likely harmless
at the time, it became a problem after 5b3c59535 added an error check
that ptype must be zero once we exit the parsing loop (that is, there
shouldn't be any unused prefixes). The consequence was that we'd
reject time or timetz input like T12:34:56 (the "extended" format
per ISO 8601-1:2019), even though that still worked in timestamp
input.
Since this is clearly under-tested code, add test cases covering all
the ISO 8601 time formats. (Note: although 8601 allows just "Thh",
we have never accepted that, and this patch doesn't change that.
I'm content to leave that as-is because it seems too likely to be
a mistake rather than intended input. If anyone wants to allow
that, it should be a separate patch anyway, and not back-patched.)
Per bug #18470 from David Perez. Back-patch to v16 where we
broke it.
Discussion: https://postgr.es/m/18470-34fad4c829106848@postgresql.org
After further review, we want to move in the direction of always
quoting GUC names in error messages, rather than the previous (PG16)
wildly mixed practice or the intermittent (mid-PG17) idea of doing
this depending on how possibly confusing the GUC name is.
This commit applies appropriate quotes to (almost?) all mentions of
GUC names in error messages. It partially supersedes a243569bf6 and
8d9978a717, which had moved things a bit in the opposite direction
but which then were abandoned in a partial state.
Author: Peter Smith <smithpb2250@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com
There were a few typedefs that were never used to define a variable or
field. This had the effect that the buildfarm's typedefs.list would
not include them, and so they would need to be re-added manually to
keep the overall pgindent result perfectly clean.
We can easily get rid of these typedefs to avoid the issue. In a few
cases, we just let the enum or struct stand on its own without a
typedef around it. In one case, an enum was abused to define flag
bits; that's better done with macro definitions.
This fixes all the remaining issues with missing entries in the
buildfarm's typedefs.list.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/1919000.1715815925@sss.pgh.pa.us
This enum was used to determine the first ID to use when assigning a
custom wait event for extensions, which is always 1. It was kept so
as it would be possible to add new in-core wait events in the category
"Extension". There is no such thing currently, so let's remove this
enum until a case justifying it pops up. This makes the code simpler
and easier to understand.
This has as effect to switch back autoprewarm.c to use PG_WAIT_EXTENSION
rather than WAIT_EVENT_EXTENSION, on par with v16 and older stable
branches.
Thinko in c9af054653.
Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/195c6c45-abce-4331-be6a-e87724e1d060@eisentraut.org
This feature set did not handle empty ranges correctly, and it's now
too late for PostgreSQL 17 to fix it.
The following commits are reverted:
6db4598fcb Add stratnum GiST support function
46a0cd4cef Add temporal PRIMARY KEY and UNIQUE constraints
86232a49a4 Fix comment on gist_stratnum_btree
030e10ff1a Rename pg_constraint.conwithoutoverlaps to conperiod
a88c800deb Use daterange and YMD in without_overlaps tests instead of tsrange.
5577a71fb0 Use half-open interval notation in without_overlaps tests
34768ee361 Add temporal FOREIGN KEY contraints
482e108cd3 Add test for REPLICA IDENTITY with a temporal key
c3db1f30cb doc: clarify PERIOD and WITHOUT OVERLAPS in CREATE TABLE
144c2ce0cc Fix ON CONFLICT DO NOTHING/UPDATE for temporal indexes
Discussion: https://www.postgresql.org/message-id/d0b64a7a-dfe4-4b84-a906-c7dedfa40a3e@eisentraut.org
