Pgq perf experiments#23
Open
NikolayS wants to merge 2730 commits into
Open
Conversation
Add a new SQL-callable function that returns the DDL statements needed to recreate a role. It takes a regrole argument and an optional VARIADIC text argument for options that are specified as alternating name/value pairs. The following options are supported: pretty (boolean) for formatted output and memberships (boolean) to include GRANT statements for role memberships and membership options. The return is one or multiple rows where the first row is a CREATE ROLE statement and subsequent rows are ALTER ROLE statements to set some role properties. Password information is never included in the output. The caller must have SELECT privilege on pg_authid. Author: Mario Gonzalez <gonzalemario@gmail.com> Author: Bryan Green <dbryan.green@gmail.com> Co-authored-by: Andrew Dunstan <andrew@dunslane.net> Co-authored-by: Euler Taveira <euler@eulerto.com> Reviewed-by: Japin Li <japinli@hotmail.com> Reviewed-by: Quan Zongliang <quanzongliang@yeah.net> Reviewed-by: jian he <jian.universality@gmail.com> Discussion: https://postgr.es/m/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com Discussion: https://postgr.es/m/e247c261-e3fb-4810-81e0-a65893170e94@dunslane.net
Add a new SQL-callable function that returns the DDL statements needed to recreate a tablespace. It takes a tablespace name or OID and an optional VARIADIC text argument for options that are specified as alternating name/value pairs. The following options are supported: pretty (boolean) for formatted output and owner (boolean) to include OWNER. (It includes two variants because there is no regtablespace pseudotype.) The return is one or multiple rows where the first row is a CREATE TABLESPACE statement and subsequent rows are ALTER TABLESPACE statements to set some tablespace properties. The caller must have SELECT privilege on pg_tablespace. get_reloptions() in ruleutils.c is made non-static so it can be called from the new ddlutils.c file. Author: Nishant Sharma <nishant.sharma@enterprisedb.com> Author: Manni Wood <manni.wood@enterprisedb.com> Co-authored-by: Andrew Dunstan <andrew@dunslane.net> Co-authored-by: Euler Taveira <euler@eulerto.com> Reviewed-by: Jim Jones <jim.jones@uni-muenster.de> Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://postgr.es/m/CAKWEB6rmnmGKUA87Zmq-s=b3Scsnj02C0kObQjnbL2ajfPWGEw@mail.gmail.com Discussion: https://postgr.es/m/e247c261-e3fb-4810-81e0-a65893170e94@dunslane.net
Add a new SQL-callable function that returns the DDL statements needed to recreate a database. It takes a regdatabase argument and an optional VARIADIC text argument for options that are specified as alternating name/value pairs. The following options are supported: pretty (boolean) for formatted output, owner (boolean) to include OWNER and tablespace (boolean) to include TABLESPACE. The return is one or multiple rows where the first row is a CREATE DATABASE statement and subsequent rows are ALTER DATABASE statements to set some database properties. The caller must have CONNECT privilege on the target database. Author: Akshay Joshi <akshay.joshi@enterprisedb.com> Co-authored-by: Andrew Dunstan <andrew@dunslane.net> Co-authored-by: Euler Taveira <euler@eulerto.com> Reviewed-by: Japin Li <japinli@hotmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de> Reviewed-by: Quan Zongliang <quanzongliang@yeah.net> Discussion: https://postgr.es/m/CANxoLDc6FHBYJvcgOnZyS+jF0NUo3Lq_83-rttBuJgs9id_UDg@mail.gmail.com Discussion: https://postgr.es/m/e247c261-e3fb-4810-81e0-a65893170e94@dunslane.net
While working on refactoring how shmem is allocated, I made a mistake where the main LWLock array did not reserve space for the LWLocks allocated with RequestNamedLWLockTranche(), and the test still passed. Matthias van de Meent spotted that before it got committed, but in order to catch such mistakes in the future, add checks in test_lwlock_tranches that the locks allocated with RequestNamedLWLockTranche() can be acquired and released. Another change is to stop requesting multiple tranches with the same name with RequestNamedLWLockTranche(). As soon as I started to test using the locks I realized that's bogus, and the next commit will forbid it. Keep test coverage for duplicates requested with LWLockNewTrancheId() for now, but make it more clear that that's what the test does. Reviewed-by: Sami Imseih <samimseih@gmail.com> Discussion: https://www.postgresql.org/message-id/463a28db-0c0b-4af6-bac6-3891828bbbfe@iki.fi Discussion: https://www.postgresql.org/message-id/CAEze2WjgCROMMXY0+j8FFdm3iFcr7By-+6Mwiz=PgGSEydiW3A@mail.gmail.com
You could request two tranches with same name, but things would get confusing when you called GetNamedLWLockTranche() to get the LWLocks allocated for them; it would always return the first tranche with the name. That doesn't make sense, so forbid duplicates. We still allow duplicates with LWLockNewTrancheId(). That works better as you don't use the name to look up the tranche ID later. It's still confusing in wait events, for example, but it's not dangerous in the same way. Reviewed-by: Sami Imseih <samimseih@gmail.com> Discussion: https://www.postgresql.org/message-id/463a28db-0c0b-4af6-bac6-3891828bbbfe@iki.fi
The two new functions allow to extract the block number and offset from a tid. There are existing ways to do so (e.g. by doing (ctid::text::point)[0]), but they are hard to remember and not pretty. tid_block() returns int8 (bigint) because BlockNumber is uint32, which exceeds the range of int4. tid_offset() returns int4 (integer) because OffsetNumber is uint16, which fits safely in int4. Bumps catversion. Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com> Discussion: https://postgr.es/m/CAJTYsWUzok2+mvSYkbVUwq_SWWg-GdHqCuYumN82AU97SjwjCA@mail.gmail.com
The database name was warned about when building with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS, leading to BF and CI failures. It is somewhat confusing that the required prefix is different for databases than other object types. Also fix a pgindent violation that caused koel to start to fail. Discussion: https://postgr.es/m/ptyiexyhmtxf4lm524s7o7w64r26ra237uusv4tjav4yhpmeoo@vfwwllz7tivb
Introduce TriggerInstrumentation to capture trigger timing and firings (previously counted in "ntuples"), to aid a future refactoring that splits out all Instrumentation fields beyond timing and WAL/buffers into more specific structs. In passing, drop the "n" argument to InstrAlloc, as all remaining callers need exactly one Instrumentation struct. The duplication between InstrAlloc() and InstrInit(), as well as the conditional initialization of async_mode will be addressed in a subsequent commit. Author: Lukas Fittl <lukas@fittl.com> Reviewed-by: Andres Freund <andres@anarazel.de> Discussion: https://www.postgresql.org/message-id/flat/CAP53PkzdBK8VJ1fS4AZ481LgMN8f9mJiC39ZRHqkFUSYq6KWmg@mail.gmail.com
Previously, different places (e.g. query "total time") were repurposing the Instrumentation struct initially introduced for capturing per-node statistics during execution. This overuse of the same struct is confusing, e.g. by cluttering calls of InstrStartNode/InstrStopNode in unrelated code paths, and prevents future refactorings. Instead, simplify the Instrumentation struct to only track time and WAL/buffer usage. Similarly, drop the use of InstrEndLoop outside of per-node instrumentation - these calls were added without any apparent benefit since the relevant fields were never read. Introduce the NodeInstrumentation struct to carry forward the per-node instrumentation information. WorkerInstrumentation is renamed to WorkerNodeInstrumentation for clarity. In passing, clarify that InstrAggNode is expected to only run after InstrEndLoop (as it does in practice), and drop unused code. This also fixes a consequence-less bug: Previously ->async_mode was only set when a non-zero instrument_option was passed. That turns out to be harmless right now, as ->async_mode only affects a timing related field. Author: Lukas Fittl <lukas@fittl.com> Reviewed-by: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/CAP53PkzdBK8VJ1fS4AZ481LgMN8f9mJiC39ZRHqkFUSYq6KWmg@mail.gmail.com
A little refactoring in preparation for the next commit, to make the material changes in that commit more clear. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
This replaces the [Subsystem]ShmemSize() and [Subsystem]ShmemInit()
functions called at postmaster startup with a new set of callbacks.
The new mechanism is designed to be more ergonomic. Notably, the size
of each shmem area is specified in the same ShmemRequestStruct() call,
together with its name. The same mechanism is used in extensions,
replacing the shmem_{request/startup}_hooks.
ShmemInitStruct() and ShmemInitHash() become backwards-compatibility
wrappers around the new functions. In future commits, I will replace
all ShmemInitStruct() and ShmemInitHash() calls with the new
functions, although we'll still need to keep them around for
extensions.
Co-authored-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
The old ShmemInit{Struct/Hash}() functions could be used after
postmaster statup, as long as the allocation is small enough to fit in
spare shmem reserved at startup. I believe some extensions do that,
although we hadn't really documented it and had not coverage for it.
The new test module covers that after-startup usage with the new
ShmemRequestStruct() functions.
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
As part of this, embed the LWLock it needs in the shared memory struct itself, so that we don't need to use RequestNamedLWLockTranche() anymore. LWLockNewTrancheId() + LWLockInitialize() is more convenient to use in extensions. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
To add a new built-in subsystem, add it to subsystemslist.h. That hooks up its shmem callbacks so that they get called at the right times during postmaster startup. For now this is unused, but will replace the current SubsystemShmemSize() and SubsystemShmemInit() calls in the next commits. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
It seems like a good candidate to convert first because it needs to initialized before any other subsystem, but other than that it's nothing special. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
These subsystems have some complicating properties, making them slightly harder to convert than most: - The initialization callbacks of some of these subsystems have dependencies, i.e. they need to be initialized in the right order. - The ProcGlobal pointer still needs to be inherited by the BackendParameters mechanism on EXEC_BACKEND builds, because ProcGlobal is required by InitProcess() to get a PGPROC entry, and the PGPROC entry is required to use LWLocks, and usually attaching to shared memory areas requires the use of LWLocks. - Similarly, ProcSignal pointer still needs to be handled by BackendParameters, because query cancellation connections access it without calling InitProcess Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
This is in preparation to convert it to use the new shmem allocation
functions, making the next commit that does that smaller. This inlines
SerialInit() to the caller, and moves all the initialization steps
within PredicateLockShmemInit() to happen after all the
ShmemInit{Struct|Hash}() calls.
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
I replaced the old SimpleLruInit() function without a backwards compatibility wrapper, because few extensions define their own SLRUs. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
This replaces the "shmem_size" and "shmem_init" callbacks in the IO methods table with the same ShmemCallback struct that we now use in other subsystems Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
The buffer blocks, converted to use ShmemRequestStruct() in the next commit, are IO-aligned. This might come handy in other places too, so make it an explicit feature of ShmemRequestStruct(). Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
This rectifies the initialization functions a little, making the "buffer strategy" stuff in freelist.c and buffer mapping hash table in buf_init.c top-level "subsystems" of their own, registered directly in subsystemlist.h. Previously they were called indirectly from BufferManagerShmemInit() and BufferManagerShmemSize() Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
This removes all remaining uses of ShmemInitStruct() and ShmemInitHash() from built-in code. Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Matthias van de Meent <boekewurm+postgres@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://www.postgresql.org/message-id/CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com
Commit 7c64d56 has removed the isolation test providing coverage for lock statistics due to some instability in the CI, where the deadlock timeout may not have enough time to process, preventing the stats data to be updated. These also relied on a set of hardcoded sleeps. This commit switches the test suite to TAP, instead, that uses an injection point with a wait to avoid the sleeps. The injection point is added in ProcSleep(), once we know that the deadlock timeout has fired and that the stats have been updated. Multiple lock patterns are checked, all rely on the same workflow, with two sessions: - session 1 holds a given lock type. - session 2 attaches to the new injection point with the wait action. - session 2 attempts to acquire a lock conflicting with the lock of session 1, waiting for the injection point to be reached. - session 1 releases its lock, session 2 commits. - pg_stat_lock is polled until the counters are updated for the lock type. Bertrand's version of the patch introduced a new routine to BackgroundPsql() to detect the blocked background sessions. I have tweaked the test so as we use the same method as some of the other tests instead, based on some \echo commands. This test has been run multiple times in the CI, all passing, so I'd like to think that this is more stable than the first version attempted. Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com> Co-authored-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/acNTR1lLHwQJ0o+P@ip-10-97-1-34.eu-west-3.compute.internal
If the background worker for processing databases manages to finish before the launcher starts waiting for it, the launcher would treat it erroneously as an error. Fix by ensureing to check result state in this case. Identified on CI and synthetically reproduced during local testing. Also while, make sure to properly lock the shared memory structure before updating tje result state. Author: Daniel Gustafsson <daniel@yesql.seA Reported-by: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/4fxw37ge47v5baeozla5phymi233hxbcjbwwsfwv3mpg3kyl2z@6jk4nkf6jp4
Postcommit review and buildfarm/CI failures revealed a few issues in
the test code which this commit attempts to resolve. These failures
are verified using synthetic means.
* Wait for launcher exit in enable/disable checksum tests
When enabling or disabling data checksums in a test with waiting
for an end state (on or off), the test typically want to perform
more test against the cluster immediately. Make sure to wait for
the launcher to exit in these cases before returning in order to
know it can immediately be acted on. This is a more generic way
of implementating 0036232.
* Refactor injection point tests to use the injection_points test
extension. Two injection points added for online checksums were
better expressed using the injection_points extension with the
test code embedded in datachecksum_state.c.
* Make tests less timing dependent and allow transitions to "on"
and not just "inprogress-on" in case a test manages to finish
before it's checked for state.
* When waiting on a blocking background psql keeping a temporary
table open, the test first closed the background session abd
then the server. This could cause data checksums to manage to
get enabled in the brief window between dropping the temporary
table and closing the server. Fix by closing the server first
before the background session.
* Remove a few superfluous duplicate checks and general cleanup
of comments as well as making LSN logging consistent.
These issues were reported by Andres as well as spotted in the
buildfarm and on CI.
Author: Daniel Gustafsson <daniel@yesql.se>
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/92F25C14-801E-4198-994D-D83E31FEB0D8@yesql.se
On MSVC Arm, USE_ARMV8_CRC32C is defined, but __builtin_constant_p is not available. Use pg_integer_constant_p and add appropriate guards. There is a similar potential hazard for the x86 path, but for now let's get the buildfarm green. Oversight in commit fbc57f2, per buildfarm member hoatzin.
…ation Previously, during shutdown, walsenders always waited until all pending data was replicated to receivers. This ensures sender and receiver stay in sync after shutdown, which is important for physical replication switchovers, but it can significantly delay shutdown. For example, in logical replication, if apply workers are blocked on locks, walsenders may wait until those locks are released, preventing shutdown from completing for a long time. This commit introduces a new GUC, wal_sender_shutdown_timeout, which specifies the maximum time a walsender waits during shutdown for all pending data to be replicated. When set, shutdown completes once all data is replicated or the timeout expires. A value of -1 (the default) disables the timeout. This can reduce shutdown time when replication is slow or stalled. However, if the timeout is reached, the sender and receiver may be left out of sync, which can be problematic for physical replication switchovers. Author: Andrey Silitskiy <a.silitskiy@postgrespro.ru> Author: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Reviewed-by: Dilip Kumar <dilipbalaut@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Andres Freund <andres@anarazel.de> Reviewed-by: Takamichi Osumi <osumi.takamichi@fujitsu.com> Reviewed-by: Peter Smith <smithpb2250@gmail.com> Reviewed-by: Greg Sabino Mullane <htamfids@gmail.com> Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com> Reviewed-by: Vitaly Davydov <v.davydov@postgrespro.ru> Reviewed-by: Ronan Dunklau <ronan@dunklau.fr> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Japin Li <japinli@hotmail.com> Reviewed-by: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/TYAPR01MB586668E50FC2447AD7F92491F5E89@TYAPR01MB5866.jpnprd01.prod.outlook.com
When determining if it is safe to use an expression as a grouping key for partial aggregation, eager aggregation relies on the B-tree equalimage support function to ensure that equality implies image equality. Previously, the code incorrectly passed the default collation of the expression's data type to the equalimage procedure, rather than the expression's actual collation. As a result, if a column used a non-deterministic collation but the base type's default collation was deterministic, eager aggregation would incorrectly assume that the column was safe for byte-level grouping. This could cause rows to be prematurely grouped and subsequently discarded by strict join conditions, resulting in incorrect query results. This patch fixes the issue by passing the expression's actual collation to the equalimage procedure. Author: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com> Discussion: https://postgr.es/m/CAMbWs48A53PY1Y4zoj7YhxPww9fO1hfnbdntKfA855zpXfVFRA@mail.gmail.com
Pushing aggregates containing volatile functions below a join can violate volatility semantics by changing the number of times the function is executed. Here we check the Aggref nodes in the targetlist and havingQual for volatile functions and disable eager aggregation when such functions are present. Author: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com> Discussion: https://postgr.es/m/CAMbWs48A53PY1Y4zoj7YhxPww9fO1hfnbdntKfA855zpXfVFRA@mail.gmail.com
…ion test Previously the stats.sql regression test used conditions like "datname = (SELECT current_database())" to check the current database name. The subquery is unnecessary, so this commit simplifies these expressions to "datname = current_database()". Author: Chao Li <lic@highgo.com> Reviewed-by: Fujii Masao <masao.fujii@gmail.com> Discussion: https://postgr.es/m/A1535A8F-65AF-4C3D-ACBE-25891CB5D38B@gmail.com
FlushUnlockedBuffer() accepted io_object and io_context arguments but hardcoded IOOBJECT_RELATION and IOCONTEXT_NORMAL when calling FlushBuffer(). Pass them through instead. Also fix FlushBuffer() to use its io_object parameter for I/O timing stats rather than hardcoding IOOBJECT_RELATION. Not actively broken since all current callers pass IOOBJECT_RELATION and IOCONTEXT_NORMAL, so not backpatched. Author: Chao Li <lic@highgo.com> Reviewed-by: Melanie Plageman <melanieplageman@gmail.com> Discussion: https://postgr.es/m/BC97546F-5C15-42F2-AD57-CFACDB9657D0@gmail.com
This neglected to set TAP_TESTS = 1, and partially compensated for that by writing duplicative hand-made rules for check and installcheck. That's not really sufficient though. The way I noticed the error was that "make distclean" didn't clean out the tmp_check subdirectory, and there might be other consequences. Do it the standard way instead.
This commit tweaks ALTER INDEX .. ATTACH PARTITION to attempt a validation of a parent index in the case where an index is already attached but the parent is not yet valid. This occurs in cases where a parent index was created invalid such as with CREATE INDEX ONLY, but was left invalid after an invalid child index was attached (partitioned indexes set indisvalid to false if at least one partition is !indisvalid, indisvalid is true in a partitioned table iff all partitions are indisvalid). This could leave a partition tree in a situation where a user could not bring the parent index back to valid after fixing the child index, as there is no built-in mechanism to do so. This commit relies on the fact that repeated ATTACH PARTITION commands on the same index silently succeed. An invalid parent index is more than just a passive issue. It causes for example ON CONFLICT on a partitioned table if the invalid parent index is used to enforce a unique constraint. Some test cases are added to track some of problematic patterns, using a set of partition trees with combinations of invalid indexes and ATTACH PARTITION. Reported-by: Mohamed Ali <moali.pg@gmail.com> Author: Sami Imseih <sanmimseih@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Haibo Yan <tristan.yim@gmail.com> Discussion: http://postgr.es/m/CAGnOmWqi1D9ycBgUeOGf6mOCd2Dcf=6sKhbf4sHLs5xAcKVCMQ@mail.gmail.com Backpatch-through: 14
The ri_FetchConstraintInfo() and ri_LoadConstraintInfo() functions were declared to return const RI_ConstraintInfo *, but callers sometimes need to modify the struct, requiring casts to drop the const. Remove the misapplied const qualifiers and the casts that worked around them. Reported-by: Peter Eisentraut <peter@eisentraut.org> Author: Peter Eisentraut <peter@eisentraut.org> Discussion: https://postgr.es/m/548600ed-8bbb-4e50-8fc3-65091b122276@eisentraut.org
If the SET or WHERE clause of an INSERT ... ON CONFLICT command references EXCLUDED.col, where col is a virtual generated column, the column was not properly expanded, leading to an "unexpected virtual generated column reference" error, or incorrect results. The problem was that expand_virtual_generated_columns() would expand virtual generated columns in both the SET and WHERE clauses and in the targetlist of the EXCLUDED pseudo-relation (exclRelTlist). Then fix_join_expr() from set_plan_refs() would turn the expanded expressions in the SET and WHERE clauses back into Vars, because they would be found to match the expression entries in the indexed tlist produced from exclRelTlist. To fix this, arrange for expand_virtual_generated_columns() to not expand virtual generated columns in exclRelTlist. This forces set_plan_refs() to resolve generation expressions in the query using non-virtual columns, as required by the executor. In addition, exclRelTlist now always contains only Vars. That was something already claimed in a couple of existing comments in the planner, which relied on that fact to skip some processing, though those did not appear to constitute active bugs. Reported-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com> Author: Satyanarayana Narlapuram <satyanarlapuram@gmail.com> Author: Dean Rasheed <dean.a.rasheed@gmail.com> Discussion: https://postgr.es/m/CAHg+QDf7wTLz_vqb1wi1EJ_4Uh+Vxm75+b4c-Ky=6P+yOAHjbQ@mail.gmail.com Backpatch-through: 18
Formerly, attempting to use WHERE CURRENT OF to update or delete from a table with virtual generated columns would fail with the error "WHERE CURRENT OF on a view is not implemented". The reason was that the check preventing WHERE CURRENT OF from being used on a view was in replace_rte_variables_mutator(), which presumed that the only way it could get there was as part of rewriting a query on a view. That is no longer the case, since replace_rte_variables() is now also used to expand the virtual generated columns of a table. Fix by doing the check for WHERE CURRENT OF on a view at parse time. This is safe, since it is no longer possible for the relkind to change after the query is parsed (as of b23cd18). Reported-by: Satyanarayana Narlapuram <satyanarlapuram@gmail.com> Author: Satyanarayana Narlapuram <satyanarlapuram@gmail.com> Author: Dean Rasheed <dean.a.rasheed@gmail.com> Discussion: https://postgr.es/m/CAHg+QDc_TwzSgb=B_QgNLt3mvZdmRK23rLb+RkanSQkDF40GjA@mail.gmail.com Backpatch-through: 18
When using ALTER TABLE ... MERGE PARTITIONS or ALTER TABLE ... SPLIT PARTITION, extension dependencies on partition indexes were being lost. This happened because the new partition indexes are created fresh from the parent partitioned table's indexes, while the old partition indexes (with their extension dependencies) are dropped. Fix this by collecting extension dependencies from source partition indexes before detaching them, then applying those dependencies to the corresponding new partition indexes after they're created. The mapping between old and new indexes is done via their common parent partitioned index. For MERGE operations, all source partition indexes sharing a parent partitioned index must have the same extension dependencies; if they differ, an error naming both conflicting partition indexes is raised. The check is implemented by collecting one entry per partition index, sorting by parent index OID, and comparing adjacent entries in a single pass. This is order-independent: the same set of partitions produces the same decision regardless of the order they are listed in the MERGE command, and subset mismatches are caught in both directions. For SPLIT operations, the new partition indexes simply inherit all extension dependencies from the source partition's index. The regression tests exercising this feature live under src/test/modules/test_extensions, where the test_ext3 and test_ext5 extensions are available; core regression tests cannot assume any particular extension is installed. Author: Matheus Alcantara <matheusssilv97@gmail.com> Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com> Reported-by: Kirill Reshke <reshkekirill@gmail.com> Reviewed-by: Dmitry Koval <d.koval@postgrespro.ru> Discussion: https://www.postgresql.org/message-id/CALdSSPjXtzGM7Uk4fWRwRMXcCczge5uNirPQcYCHKPAWPkp9iQ%40mail.gmail.com
This function writes into a caller-supplied buffer of length 2 * MAXNORMLEN, which should be plenty in real-world cases. However a malicious affix file could supply an affix long enough to overrun that. Defend by just rejecting the match if it would overrun the buffer. I also inserted a check of the input word length against Affix->replen, just to be sure we won't index off the buffer, though it would be caller error for that not to be true. Also make the actual copying steps a bit more readable, and remove an unnecessary requirement for the whole input word to fit into the output buffer (even though it always will with the current caller). The lack of documentation in this code makes my head hurt, so I also reverse-engineered a basic header comment for CheckAffix. Reported-by: Xint Code Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/641711.1776792744@sss.pgh.pa.us Backpatch-through: 14
parse_affentry() and addCompoundAffixFlagValue() each collect fields from an affix file into working buffers of size BUFSIZ. They failed to defend against overlength fields, so that a malicious affix file could cause a stack smash. BUFSIZ (typically 8K) is certainly way longer than any reasonable affix field, but let's fix this while we're closing holes in this area. I chose to do this by silently truncating the input before it can overrun the buffer, using logic comparable to the existing logic in get_nextfield(). Certainly there's at least as good an argument for raising an error, but for now let's follow the existing precedent. Reported-by: Igor Stepansky <igor.stepansky@orca.security> Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/864123.1776810909@sss.pgh.pa.us Backpatch-through: 14
to_char() allocates its output buffer with 8 bytes per formatting code in the pattern. If the locale's currency symbol, thousands separator, or decimal or sign symbol is more than 8 bytes long, in principle we could overrun the output buffer. No such locales exist in the real world, so it seems sufficient to truncate the symbol if we do see it's too long. Reported-by: Xint Code Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/638232.1776790821@sss.pgh.pa.us Backpatch-through: 14
Make sure that function declarations use names that exactly match the corresponding names from function definitions in a few places. Most of these inconsistencies were introduced during Postgres 19 development. 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 035ce1f).
Commit 7a1f0f8 optimized the slot verification query but overlooked cases where all logical replication slots are already invalidated. In this scenario, the CTE returns no rows, causing the main query (which used a cross join) to return an empty result even when invalid slots exist. This commit fixes this by using a LEFT JOIN with the CTE, ensuring that slots are properly reported even if the CTE returns no rows. Author: Lakshmi N <lakshmin.jhs@gmail.com> Reviewed-by: Shveta Malik <shveta.malik@gmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://postgr.es/m/CA+3i_M8eT6j8_cBHkYykV-SXCxbmAxpVSKptjDVq+MFtpT-Paw@mail.gmail.com
The problem report was about setting GUCs in the startup packet for a physical replication connection. Setting the GUC required an ACL check, which performed a lookup on pg_parameter_acl.parname. The catalog cache was hardwired to use DEFAULT_COLLATION_OID for texteqfast() and texthashfast(), but the database default collation was uninitialized because it's a physical walsender and never connects to a database. In versions 18 and later, this resulted in a NULL pointer dereference, while in version 17 it resulted in an ERROR. As the comments stated, using DEFAULT_COLLATION_OID was arbitrary anyway: if the collation actually mattered, it should have used the column's actual collation. (In the catalog, some text columns are the default collation and some are "C".) Fix by using C_COLLATION_OID, which doesn't require any initialization and is always available. When any deterministic collation will do, it's best to consistently use the simplest and fastest one, so this is a good idea anyway. Another problem was raised in the thread, which this commit doesn't fix (see second discussion link). Reported-by: Andrey Borodin <x4mmm@yandex-team.ru> Discussion: https://postgr.es/m/D18AD72A-5004-4EF8-AF80-10732AF677FA@yandex-team.ru Discussion: https://postgr.es/m/4524ed61a015d3496fc008644dcb999bb31916a7.camel%40j-davis.com Backpatch-through: 17
NikolayS
force-pushed
the
pgq-perf-experiments
branch
from
9bec717 to
d45d100
Compare
Add wait__event__start and wait__event__end probes to the DTrace
provider definition and invoke them from the static inline functions
pgstat_report_wait_start() and pgstat_report_wait_end().
Because these functions are static inline, they get inlined at every
call site (~100 locations across 36 files), leaving no function symbol
for eBPF uprobes to attach to. USDT probes solve this: the compiler
emits a nop instruction at each inlined site with ELF .note.stapsdt
metadata, allowing eBPF tools to discover and attach to all call sites
with a single probe definition.
This enables full eBPF-based wait event tracing (e.g., with bpftrace)
without requiring hardware watchpoints or PostgreSQL source patches
beyond this change.
When built without --enable-dtrace, the probes compile to do {} while(0)
with zero overhead.
PoC: covers all wait events via the two central inline functions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 ideas ranked by confidence (9/10 to 1/10) based on profiling PgQ insert_event() at ~148k ev/s on PG 18.3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
With 10+ concurrent inserters (common in queue workloads like PgQ), 8 WAL insertion locks become a serialization bottleneck. Raising to 32 spreads inserters across more locks, reducing LWLock:WALInsert wait time. The tradeoff is that WAL flush must iterate all locks, but 32 iterations vs 8 is negligible compared to the I/O cost of flushing. The additional shared memory is only ~3 KB (24 extra 128-byte WALInsertLockPadded structs). All usages of NUM_XLOGINSERT_LOCKS are via the macro (modulo for lock selection, loop bounds, shared memory sizing) — no hardcoded assumptions about the value being 8. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Detailed design analysis for spreading concurrent inserters across N target blocks (smgr_targblock[4]) instead of one, hashing by MyProcNumber. Documents data structure changes, code changes (~30 LOC), all interaction risks (FSM, VACUUM, visibility map, COPY/bistate, extension lock, smgr invalidation), and a gated prototype plan that depends on benchmarking Ideas 3+4 first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a BulkInsertState to the ModifyTable executor node so that INSERT statements going through the standard executor path get the same bulk-extend and FSM-bypass optimizations that COPY uses. Previously, only COPY created a BulkInsertState; the executor INSERT path always passed bistate=NULL to table_tuple_insert(), meaning: - No adaptive bulk extension (already_extended_by was never tracked) - Pre-extended pages went into the FSM where other backends could grab them, wasting the extension effort - No buffer pin caching across consecutive inserts This change creates a BulkInsertState in ExecInitModifyTable() for non-FDW INSERT operations and passes it through to table_tuple_insert() in ExecInsert(). For partitioned tables, ReleaseBulkInsertStatePin() is called when the target partition changes, following the same pattern COPY uses. The main beneficiary is INSERT ... SELECT (multi-row inserts within a single plan execution). For single-row SPI INSERTs (e.g., PgQ), the bistate is created/destroyed per plan execution, so adaptive scaling does not accumulate across calls — a future Phase 2 optimization would persist the bistate across SPI calls within an SPI connection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion Change smgr_targblock from a single BlockNumber to an array of 4 slots. Each backend hashes into its own slot via (MyProcNumber & 3), so concurrent inserters naturally target different heap pages instead of all fighting for exclusive BufferContent lock on the same 8KB page. This addresses the ~23% LWLock:BufferContent contention seen in profiling of append-heavy queue workloads at 10+ concurrent inserters. Changes: - smgr.h: Add SMGR_TARGBLOCK_SLOTS (4), change smgr_targblock to array - rel.h: Add RelationTargetBlockSlot() macro, update RelationGetTargetBlock and RelationSetTargetBlock to index by backend slot - smgr.c: Initialize all 4 slots to InvalidBlockNumber in smgropen() and smgrrelease() - storage.c: Same initialization fix in RelationTruncate() The existing Assert sites in createas.c, matview.c, and heapam_handler.c work unchanged because they check the current backend's slot, which will be InvalidBlockNumber when no writes have occurred to a freshly created relation. BulkInsertState (COPY) bypasses target blocks entirely via bistate->current_buf, so COPY is unaffected. Memory overhead: 12 bytes extra per SMgrRelation entry (3 additional BlockNumber slots). Full build passes with zero warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A/B test stock PG 18 vs patched PG 19dev with: - NUM_XLOGINSERT_LOCKS 8→32 - BulkInsertState for executor INSERT - Multi-slot target blocks (4 slots) Peak: 193k ev/s (100B) / 120k ev/s = 235 MB/s (2KB) Biggest win: multi-target blocks at 16+ clients nearly doubles small-payload throughput. Also tested wal_compression (lz4, zstd) — hurts with random JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lysis - 8 target slots regresses 19.5% vs 4 — cache pressure, reverted - PL/pgSQL mode: 95-125k ev/s, 73-82% of C at low concurrency - Sequence contention not visible — Lock:extend is the real bottleneck - AIO writes not available yet (reads only in PG19dev) - IO:DataFileWrite at 51% is the ceiling — needs async writes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tested PG 19dev AIO settings: - io_max_concurrency=128: +40% individually but unreliable in combo - debug_io_direct='data': +26% individually, conflicts with other settings - effective_io_concurrency=200: +35% individually - Combined: worse than individual (settings fight each other) Also tested: - TOAST: non-factor (2KB stays inline) - ev_txid index: 3% overhead (acceptable for consumer reads) - Sequence cache=100: 0.9% (noise) Full summary table of all 14 optimization attempts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Key finding: PG 19dev unpatched is SLOWER than PG 18 at low concurrency (likely debug/AIO overhead). Our 3 patches provide +98% to +264% improvement on PG 19dev baseline. commit_delay=50 with commit_siblings=3 gives +25% for free (config only). Updated full optimization inventory (14 items tested). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Key findings: - wal_compression=lz4 HELPS (+5.6%) with compressible JSON — earlier "hurts" finding was measurement noise. Now RECOMMENDED. - Full cycle: 1K batch 53% faster on patched PG, consumer ops sub-ms - commit_delay inconclusive on macOS (too much variance) - 16 total optimizations tested across 5 rounds Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Final results on stock PG 18.3 with proven config tuning: - PL/pgSQL (pgq2): 88k ev/s (1KB JSON), 75k ev/s (2KB) = 147 MB/s - C mode: 161k ev/s (100B), 135k ev/s (1KB) = 141 MB/s - Full cycle: 145ms for 10K events end-to-end - Consumer ops (tick/next/finish): 1-3ms constant - Tuning guide: synchronous_commit=off, shared_buffers, max_wal_size, lz4 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Final round: fill-then-drain test shows consumer is NOT the bottleneck (33K ev/s for giant 4.87M-event batch, 302K ev/s for normal batches). 7 rounds, 16 optimizations tested. Practical limit reached for this hardware. Final numbers and recommended tuning guide documented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
NikolayS
force-pushed
the
pgq-perf-experiments
branch
from
d45d100 to
e8f8754
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.