public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5 1/2] In-place table persistence change
7+ messages / 5 participants
[nested] [flat]
* [PATCH v5 1/2] In-place table persistence change
@ 2020-11-11 12:51 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Kyotaro Horiguchi @ 2020-11-11 12:51 UTC (permalink / raw)
Even though ALTER TABLE SET LOGGED/UNLOGGED does not require data
rewriting, currently it runs heap rewrite which causes large amount of
file I/O. This patch makes the command run without heap rewrite.
Addition to that, SET LOGGED while wal_level > minimal emits WAL using
XLOG_FPI instead of massive number of HEAP_INSERT's, which should be
smaller.
Also this allows for the cleanup of files left behind in the crash of
the transaction that created it.
---
src/backend/access/rmgrdesc/smgrdesc.c | 23 ++
src/backend/access/transam/README | 8 +
src/backend/access/transam/xlog.c | 17 +
src/backend/catalog/storage.c | 436 +++++++++++++++++++++++--
src/backend/commands/tablecmds.c | 246 +++++++++++---
src/backend/storage/buffer/bufmgr.c | 88 +++++
src/backend/storage/file/reinit.c | 316 ++++++++++++------
src/backend/storage/smgr/md.c | 13 +-
src/backend/storage/smgr/smgr.c | 6 +
src/common/relpath.c | 4 +-
src/include/catalog/storage.h | 2 +
src/include/catalog/storage_xlog.h | 22 +-
src/include/common/relpath.h | 6 +-
src/include/storage/bufmgr.h | 2 +
src/include/storage/md.h | 2 +
src/include/storage/reinit.h | 3 +-
src/include/storage/smgr.h | 1 +
17 files changed, 1028 insertions(+), 167 deletions(-)
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index 7755553d57..2c109b8ca4 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -40,6 +40,23 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
xlrec->blkno, xlrec->flags);
pfree(path);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) rec;
+ char *path = relpathperm(xlrec->rnode, xlrec->forkNum);
+
+ appendStringInfoString(buf, path);
+ pfree(path);
+ }
+ else if (info == XLOG_SMGR_BUFPERSISTENCE)
+ {
+ xl_smgr_bufpersistence *xlrec = (xl_smgr_bufpersistence *) rec;
+ char *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
+
+ appendStringInfoString(buf, path);
+ appendStringInfo(buf, " persistence %d", xlrec->persistence);
+ pfree(path);
+ }
}
const char *
@@ -55,6 +72,12 @@ smgr_identify(uint8 info)
case XLOG_SMGR_TRUNCATE:
id = "TRUNCATE";
break;
+ case XLOG_SMGR_UNLINK:
+ id = "UNLINK";
+ break;
+ case XLOG_SMGR_BUFPERSISTENCE:
+ id = "BUFPERSISTENCE";
+ break;
}
return id;
diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index 1edc8180c1..547107a771 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -724,6 +724,14 @@ we must panic and abort recovery. The DBA will have to manually clean up
then restart recovery. This is part of the reason for not writing a WAL
entry until we've successfully done the original action.
+The CLEANUP fork file
+--------------------------------
+
+An CLEANUP fork is created when a new relation file is created to mark
+the relfilenode needs to be cleaned up at recovery time. In contrast
+to 4 above, failure to remove an CLEANUP fork file will lead to data
+loss, in which case the server will shut down.
+
Skipping WAL for New RelFileNode
--------------------------------
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b18257c198..6dcbcbe387 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -40,6 +40,7 @@
#include "catalog/catversion.h"
#include "catalog/pg_control.h"
#include "catalog/pg_database.h"
+#include "catalog/storage.h"
#include "commands/progress.h"
#include "commands/tablespace.h"
#include "common/controldata_utils.h"
@@ -4442,6 +4443,14 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
{
ereport(DEBUG1,
(errmsg_internal("reached end of WAL in pg_wal, entering archive recovery")));
+
+ /* cleanup garbage files left during crash recovery */
+ ResetUnloggedRelations(UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_CLEANUP);
+
+ /* run rollback cleanup if any */
+ smgrDoPendingDeletes(false);
+
InArchiveRecovery = true;
if (StandbyModeRequested)
StandbyMode = true;
@@ -7455,6 +7464,14 @@ StartupXLOG(void)
}
}
+ /* cleanup garbage files left during crash recovery */
+ if (!InArchiveRecovery)
+ ResetUnloggedRelations(UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_CLEANUP);
+
+ /* run rollback cleanup if any */
+ smgrDoPendingDeletes(false);
+
/* Allow resource managers to do any required cleanup. */
for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
{
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index cba7a9ada0..c54d70747f 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -19,6 +19,7 @@
#include "postgres.h"
+#include "access/amapi.h"
#include "access/parallel.h"
#include "access/visibilitymap.h"
#include "access/xact.h"
@@ -27,6 +28,7 @@
#include "access/xlogutils.h"
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
+#include "common/hashfn.h"
#include "miscadmin.h"
#include "storage/freespace.h"
#include "storage/smgr.h"
@@ -57,9 +59,16 @@ int wal_skip_threshold = 2048; /* in kilobytes */
* but I'm being paranoid.
*/
+#define PDOP_DELETE (0)
+#define PDOP_UNLINK_FORK (1 << 0)
+#define PDOP_SET_PERSISTENCE (1 << 1)
+
typedef struct PendingRelDelete
{
RelFileNode relnode; /* relation that may need to be deleted */
+ int op; /* operation mask */
+ bool bufpersistence; /* buffer persistence to set */
+ int unlink_forknum; /* forknum to unlink */
BackendId backend; /* InvalidBackendId if not a temp rel */
bool atCommit; /* T=delete at commit; F=delete at abort */
int nestLevel; /* xact nesting level of request */
@@ -75,6 +84,24 @@ typedef struct PendingRelSync
static PendingRelDelete *pendingDeletes = NULL; /* head of linked list */
HTAB *pendingSyncHash = NULL;
+typedef struct SRelHashEntry
+{
+ SMgrRelation srel;
+ char status; /* for simplehash use */
+} SRelHashEntry;
+
+/* define hashtable for workarea for pending deletes */
+#define SH_PREFIX srelhash
+#define SH_ELEMENT_TYPE SRelHashEntry
+#define SH_KEY_TYPE SMgrRelation
+#define SH_KEY srel
+#define SH_HASH_KEY(tb, key) \
+ hash_bytes((unsigned char *)&key, sizeof(SMgrRelation))
+#define SH_EQUAL(tb, a, b) (memcmp(&a, &b, sizeof(SMgrRelation)) == 0)
+#define SH_SCOPE static inline
+#define SH_DEFINE
+#define SH_DECLARE
+#include "lib/simplehash.h"
/*
* AddPendingSync
@@ -143,7 +170,17 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
return NULL; /* placate compiler */
}
+ /*
+ * We are going to create a new storage file. If server crashes before the
+ * current transaction ends the file needs to be cleaned up but there's no
+ * clue to the orphan files. The cleanup fork works as the sentinel to
+ * identify that situation.
+ */
srel = smgropen(rnode, backend);
+ smgrcreate(srel, CLEANUP2_FORKNUM, false);
+ log_smgrcreate(&rnode, CLEANUP2_FORKNUM);
+ smgrimmedsync(srel, CLEANUP2_FORKNUM);
+
smgrcreate(srel, MAIN_FORKNUM, false);
if (needs_wal)
@@ -153,12 +190,25 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rnode;
+ pending->op = PDOP_DELETE;
pending->backend = backend;
pending->atCommit = false; /* delete if abort */
pending->nestLevel = GetCurrentTransactionNestLevel();
pending->next = pendingDeletes;
pendingDeletes = pending;
+ /* drop cleanup fork at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK;
+ pending->unlink_forknum = CLEANUP2_FORKNUM;
+ pending->backend = backend;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
if (relpersistence == RELPERSISTENCE_PERMANENT && !XLogIsNeeded())
{
Assert(backend == InvalidBackendId);
@@ -168,6 +218,218 @@ RelationCreateStorage(RelFileNode rnode, char relpersistence)
return srel;
}
+/*
+ * RelationCreateInitFork
+ * Create physical storage for the init fork of a relation.
+ *
+ * Create the init fork for the relation.
+ *
+ * This function is transactional. The creation is WAL-logged, and if the
+ * transaction aborts later on, the init fork will be removed.
+ */
+void
+RelationCreateInitFork(Relation rel)
+{
+ RelFileNode rnode = rel->rd_node;
+ PendingRelDelete *pending;
+ SMgrRelation srel;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+ bool create = true;
+
+ /* switch buffer persistence */
+ SetRelationBuffersPersistence(rel->rd_smgr, false, false);
+
+ /*
+ * If we have entries for init-fork operation of this relation, that means
+ * that we have already registered pending delete entries to drop
+ * preexisting init fork since before the current transaction started. This
+ * function reverts that change just by removing the entries.
+ */
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->op != PDOP_DELETE &&
+ ((pending->op & PDOP_UNLINK_FORK) != 0 &&
+ pending->unlink_forknum == CLEANUP_FORKNUM))
+ {
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+
+ create = false;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+
+ if (!create)
+ return;
+
+ /*
+ * We are going to create the init fork. If server crashes before the
+ * current transaction ends the init fork left alone corrupts data while
+ * recovery. The cleanup fork works as the sentinel to identify that
+ * situation.
+ */
+ srel = smgropen(rnode, InvalidBackendId);
+ smgrcreate(srel, CLEANUP_FORKNUM, false);
+ log_smgrcreate(&rnode, CLEANUP_FORKNUM);
+ smgrimmedsync(srel, CLEANUP_FORKNUM);
+
+ /* We don't have existing init fork, create it. */
+ smgrcreate(srel, INIT_FORKNUM, false);
+
+ /*
+ * index-init fork needs further initialization. ambuildempty shoud do
+ * WAL-log and file sync by itself but otherwise we do that by myself.
+ */
+ if (rel->rd_rel->relkind == RELKIND_INDEX)
+ rel->rd_indam->ambuildempty(rel);
+ else
+ {
+ log_smgrcreate(&rnode, INIT_FORKNUM);
+ smgrimmedsync(srel, INIT_FORKNUM);
+ }
+
+ /* drop this init fork file at abort and revert persistence */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK | PDOP_SET_PERSISTENCE;
+ pending->unlink_forknum = INIT_FORKNUM;
+ pending->bufpersistence = true;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
+ /* drop cleanup fork at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK;
+ pending->unlink_forknum = CLEANUP_FORKNUM;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
+ /* drop cleanup fork at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK;
+ pending->unlink_forknum = CLEANUP_FORKNUM;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
+/*
+ * RelationDropInitFork
+ * Delete physical storage for the init fork of a relation.
+ *
+ * Register pending-delete of the init fork. The real deletion is performed by
+ * smgrDoPendingDeletes at commit.
+ *
+ * This function is transactional. If the transaction aborts later on, the
+ * deletion doesn't happen.
+ */
+void
+RelationDropInitFork(Relation rel)
+{
+ RelFileNode rnode = rel->rd_node;
+ PendingRelDelete *pending;
+ PendingRelDelete *prev;
+ PendingRelDelete *next;
+ bool inxact_created = false;
+
+ /* switch buffer persistence */
+ SetRelationBuffersPersistence(rel->rd_smgr, true, false);
+
+ /*
+ * If we have entries for init-fork operation of this relation, that means
+ * that we have created the init fork in the current transaction. We
+ * immediately remove the init and cleanup forks immediately in that case.
+ * Otherwise just reister pending-delete for the existing init fork.
+ */
+ prev = NULL;
+ for (pending = pendingDeletes; pending != NULL; pending = next)
+ {
+ next = pending->next;
+ if (RelFileNodeEquals(rnode, pending->relnode) &&
+ pending->op != PDOP_DELETE &&
+ ((pending->op & PDOP_UNLINK_FORK) != 0 &&
+ pending->unlink_forknum == CLEANUP_FORKNUM))
+ {
+ /* unlink list entry */
+ if (prev)
+ prev->next = next;
+ else
+ pendingDeletes = next;
+ pfree(pending);
+
+ inxact_created = true;
+ }
+ else
+ {
+ /* unrelated entry, don't touch it */
+ prev = pending;
+ }
+ }
+
+ if (inxact_created)
+ {
+ SMgrRelation srel = smgropen(rnode, InvalidBackendId);
+
+ /*
+ * INIT/CLEANUP forks never be loaded to shared buffer so no point in
+ * dropping buffers for these files.
+ */
+ log_smgrunlink(&rnode, INIT_FORKNUM);
+ smgrunlink(srel, INIT_FORKNUM, false);
+ log_smgrunlink(&rnode, CLEANUP_FORKNUM);
+ smgrunlink(srel, CLEANUP_FORKNUM, false);
+ return;
+ }
+
+ /* register drop of this init fork file at commit */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_UNLINK_FORK;
+ pending->unlink_forknum = INIT_FORKNUM;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = true;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+
+ /* revert buffer-persistence changes at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = rnode;
+ pending->op = PDOP_SET_PERSISTENCE;
+ pending->bufpersistence = false;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+}
+
/*
* Perform XLogInsert of an XLOG_SMGR_CREATE record to WAL.
*/
@@ -187,6 +449,44 @@ log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum)
XLogInsert(RM_SMGR_ID, XLOG_SMGR_CREATE | XLR_SPECIAL_REL_UPDATE);
}
+/*
+ * Perform XLogInsert of an XLOG_SMGR_UNLINK record to WAL.
+ */
+void
+log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum)
+{
+ xl_smgr_unlink xlrec;
+
+ /*
+ * Make an XLOG entry reporting the file unlink.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.forkNum = forkNum;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_UNLINK | XLR_SPECIAL_REL_UPDATE);
+}
+
+/*
+ * Perform XLogInsert of an XLOG_SMGR_BUFPERSISTENCE record to WAL.
+ */
+void
+log_smgrbufpersistence(const RelFileNode *rnode, bool persistence)
+{
+ xl_smgr_bufpersistence xlrec;
+
+ /*
+ * Make an XLOG entry reporting the change of buffer persistence.
+ */
+ xlrec.rnode = *rnode;
+ xlrec.persistence = persistence;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, sizeof(xlrec));
+ XLogInsert(RM_SMGR_ID, XLOG_SMGR_BUFPERSISTENCE | XLR_SPECIAL_REL_UPDATE);
+}
+
/*
* RelationDropStorage
* Schedule unlinking of physical storage at transaction commit.
@@ -200,6 +500,7 @@ RelationDropStorage(Relation rel)
pending = (PendingRelDelete *)
MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
pending->relnode = rel->rd_node;
+ pending->op = PDOP_DELETE;
pending->backend = rel->rd_backend;
pending->atCommit = true; /* delete if commit */
pending->nestLevel = GetCurrentTransactionNestLevel();
@@ -602,59 +903,97 @@ smgrDoPendingDeletes(bool isCommit)
int nrels = 0,
maxrels = 0;
SMgrRelation *srels = NULL;
+ srelhash_hash *close_srels = NULL;
+ bool found;
prev = NULL;
for (pending = pendingDeletes; pending != NULL; pending = next)
{
+ SMgrRelation srel;
+
next = pending->next;
if (pending->nestLevel < nestLevel)
{
/* outer-level entries should not be processed yet */
prev = pending;
+ continue;
}
+
+ /* unlink list entry first, so we don't retry on failure */
+ if (prev)
+ prev->next = next;
else
+ pendingDeletes = next;
+
+ if (pending->atCommit != isCommit)
{
- /* unlink list entry first, so we don't retry on failure */
- if (prev)
- prev->next = next;
- else
- pendingDeletes = next;
- /* do deletion if called for */
- if (pending->atCommit == isCommit)
- {
- SMgrRelation srel;
-
- srel = smgropen(pending->relnode, pending->backend);
-
- /* allocate the initial array, or extend it, if needed */
- if (maxrels == 0)
- {
- maxrels = 8;
- srels = palloc(sizeof(SMgrRelation) * maxrels);
- }
- else if (maxrels <= nrels)
- {
- maxrels *= 2;
- srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
- }
-
- srels[nrels++] = srel;
- }
/* must explicitly free the list entry */
pfree(pending);
/* prev does not change */
+ continue;
+ }
+
+ if (close_srels == NULL)
+ close_srels = srelhash_create(CurrentMemoryContext, 32, NULL);
+
+ srel = smgropen(pending->relnode, pending->backend);
+
+ /* Uniquify the smgr relations */
+ srelhash_insert(close_srels, srel, &found);
+
+ if (pending->op != PDOP_DELETE)
+ {
+ if (pending->op & PDOP_UNLINK_FORK)
+ {
+ /* other forks needs to drop buffers */
+ Assert(pending->unlink_forknum == INIT_FORKNUM ||
+ pending->unlink_forknum == CLEANUP_FORKNUM ||
+ pending->unlink_forknum == CLEANUP2_FORKNUM);
+
+ log_smgrunlink(&pending->relnode, pending->unlink_forknum);
+ smgrunlink(srel, pending->unlink_forknum, false);
+
+ }
+
+ if (pending->op & PDOP_SET_PERSISTENCE)
+ SetRelationBuffersPersistence(srel, pending->bufpersistence,
+ InRecovery);
+ }
+ else
+ {
+ /* allocate the initial array, or extend it, if needed */
+ if (maxrels == 0)
+ {
+ maxrels = 8;
+ srels = palloc(sizeof(SMgrRelation) * maxrels);
+ }
+ else if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
+
+ srels[nrels++] = srel;
}
}
if (nrels > 0)
{
smgrdounlinkall(srels, nrels, false);
-
- for (int i = 0; i < nrels; i++)
- smgrclose(srels[i]);
-
pfree(srels);
}
+
+ if (close_srels)
+ {
+ srelhash_iterator i;
+ SRelHashEntry *ent;
+
+ /* close smgr relatoins */
+ srelhash_start_iterate(close_srels, &i);
+ while ((ent = srelhash_iterate(close_srels, &i)) != NULL)
+ smgrclose(ent->srel);
+ srelhash_destroy(close_srels);
+ }
}
/*
@@ -824,7 +1163,8 @@ smgrGetPendingDeletes(bool forCommit, RelFileNode **ptr)
for (pending = pendingDeletes; pending != NULL; pending = pending->next)
{
if (pending->nestLevel >= nestLevel && pending->atCommit == forCommit
- && pending->backend == InvalidBackendId)
+ && pending->backend == InvalidBackendId
+ && pending->op == PDOP_DELETE)
nrels++;
}
if (nrels == 0)
@@ -837,7 +1177,8 @@ smgrGetPendingDeletes(bool forCommit, RelFileNode **ptr)
for (pending = pendingDeletes; pending != NULL; pending = pending->next)
{
if (pending->nestLevel >= nestLevel && pending->atCommit == forCommit
- && pending->backend == InvalidBackendId)
+ && pending->backend == InvalidBackendId &&
+ pending->op == PDOP_DELETE)
{
*rptr = pending->relnode;
rptr++;
@@ -917,6 +1258,15 @@ smgr_redo(XLogReaderState *record)
reln = smgropen(xlrec->rnode, InvalidBackendId);
smgrcreate(reln, xlrec->forkNum, true);
}
+ else if (info == XLOG_SMGR_UNLINK)
+ {
+ xl_smgr_unlink *xlrec = (xl_smgr_unlink *) XLogRecGetData(record);
+ SMgrRelation reln;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ smgrunlink(reln, xlrec->forkNum, true);
+ smgrclose(reln);
+ }
else if (info == XLOG_SMGR_TRUNCATE)
{
xl_smgr_truncate *xlrec = (xl_smgr_truncate *) XLogRecGetData(record);
@@ -1005,6 +1355,28 @@ smgr_redo(XLogReaderState *record)
FreeFakeRelcacheEntry(rel);
}
+ else if (info == XLOG_SMGR_BUFPERSISTENCE)
+ {
+ xl_smgr_bufpersistence *xlrec =
+ (xl_smgr_bufpersistence *) XLogRecGetData(record);
+ SMgrRelation reln;
+ PendingRelDelete *pending;
+
+ reln = smgropen(xlrec->rnode, InvalidBackendId);
+ SetRelationBuffersPersistence(reln, xlrec->persistence, true);
+
+ /* revert buffer-persistence changes at abort */
+ pending = (PendingRelDelete *)
+ MemoryContextAlloc(TopMemoryContext, sizeof(PendingRelDelete));
+ pending->relnode = xlrec->rnode;
+ pending->op = PDOP_SET_PERSISTENCE;
+ pending->bufpersistence = !xlrec->persistence;
+ pending->backend = InvalidBackendId;
+ pending->atCommit = false;
+ pending->nestLevel = GetCurrentTransactionNestLevel();
+ pending->next = pendingDeletes;
+ pendingDeletes = pending;
+ }
else
elog(PANIC, "smgr_redo: unknown op code %u", info);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 993da56d43..37a15d31ee 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -50,6 +50,7 @@
#include "commands/defrem.h"
#include "commands/event_trigger.h"
#include "commands/policy.h"
+#include "commands/progress.h"
#include "commands/sequence.h"
#include "commands/tablecmds.h"
#include "commands/tablespace.h"
@@ -4917,6 +4918,170 @@ ATParseTransformCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
return newcmd;
}
+/*
+ * RelationChangePersistence: do in-place persistence change of a relation
+ */
+static void
+RelationChangePersistence(AlteredTableInfo *tab, char persistence,
+ LOCKMODE lockmode)
+{
+ Relation rel;
+ Relation classRel;
+ HeapTuple tuple,
+ newtuple;
+ Datum new_val[Natts_pg_class];
+ bool new_null[Natts_pg_class],
+ new_repl[Natts_pg_class];
+ int i;
+ List *relids;
+ ListCell *lc_oid;
+
+ Assert(tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE);
+ Assert(lockmode == AccessExclusiveLock);
+
+ /*
+ * Under the following condition, we need to call ATRewriteTable, which
+ * cannot be false in the AT_REWRITE_ALTER_PERSISTENCE case.
+ */
+ Assert(tab->constraints == NULL && tab->partition_constraint == NULL &&
+ tab->newvals == NULL && !tab->verify_new_notnull);
+
+ rel = table_open(tab->relid, lockmode);
+
+ Assert(rel->rd_rel->relpersistence != persistence);
+
+ elog(DEBUG1, "perform im-place persistnce change");
+
+ RelationOpenSmgr(rel);
+
+ /*
+ * First we collect all relations that we need to change persistence.
+ */
+
+ /* Collect OIDs of indexes and toast relations */
+ relids = RelationGetIndexList(rel);
+ relids = lcons_oid(rel->rd_id, relids);
+
+ /* Add toast relation if any */
+ if (OidIsValid(rel->rd_rel->reltoastrelid))
+ {
+ List *toastidx;
+ Relation toastrel = table_open(rel->rd_rel->reltoastrelid, lockmode);
+
+ RelationOpenSmgr(toastrel);
+ relids = lappend_oid(relids, rel->rd_rel->reltoastrelid);
+ toastidx = RelationGetIndexList(toastrel);
+ relids = list_concat(relids, toastidx);
+ pfree(toastidx);
+ table_close(toastrel, NoLock);
+ }
+
+ table_close(rel, NoLock);
+
+ /* Make changes in storage */
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+
+ foreach (lc_oid, relids)
+ {
+ Oid reloid = lfirst_oid(lc_oid);
+ Relation r = relation_open(reloid, lockmode);
+
+ /*
+ * Some access methods do not accept in-place persistence change. For
+ * example, GiST uses page LSNs to figure out whether a block has
+ * changed, where UNLOGGED GiST indexes use fake LSNs that are
+ * incompatible with real LSNs used for LOGGED ones.
+ *
+ * XXXX: We don't bother allowing in-place persistence change for index
+ * methods other than btree for now.
+ */
+ if (r->rd_rel->relkind == RELKIND_INDEX &&
+ r->rd_rel->relam != BTREE_AM_OID)
+ {
+ int reindex_flags;
+
+ /* reindex doesn't allow concurrent use of the index */
+ table_close(r, NoLock);
+
+ reindex_flags =
+ REINDEX_REL_SUPPRESS_INDEX_USE |
+ REINDEX_REL_CHECK_CONSTRAINTS;
+
+ /* Set the same persistence with the parent relation. */
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ reindex_flags |= REINDEX_REL_FORCE_INDEXES_UNLOGGED;
+ else
+ reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
+
+ reindex_index(reloid, reindex_flags, persistence, 0);
+
+ continue;
+ }
+
+ RelationOpenSmgr(r);
+
+ /* Create or drop init fork */
+ if (persistence == RELPERSISTENCE_UNLOGGED)
+ RelationCreateInitFork(r);
+ else
+ RelationDropInitFork(r);
+
+ /*
+ * When this relation gets WAL-logged, immediately sync all files but
+ * initfork to establish the initial state on storage. Buffers have
+ * already flushed out by RelationCreate(Drop)InitFork called just
+ * above. Initfork should have been synced as needed.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT)
+ {
+ for (i = 0 ; i < INIT_FORKNUM ; i++)
+ {
+ if (smgrexists(r->rd_smgr, i))
+ smgrimmedsync(r->rd_smgr, i);
+ }
+ }
+
+ /* Update catalog */
+ tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", reloid);
+
+ memset(new_val, 0, sizeof(new_val));
+ memset(new_null, false, sizeof(new_null));
+ memset(new_repl, false, sizeof(new_repl));
+
+ new_val[Anum_pg_class_relpersistence - 1] = CharGetDatum(persistence);
+ new_null[Anum_pg_class_relpersistence - 1] = false;
+ new_repl[Anum_pg_class_relpersistence - 1] = true;
+
+ newtuple = heap_modify_tuple(tuple, RelationGetDescr(classRel),
+ new_val, new_null, new_repl);
+
+ CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple);
+ heap_freetuple(newtuple);
+
+ /*
+ * While wal_level >= replica, switching to LOGGED requires the
+ * relation content to be WAL-logged to recover the table.
+ */
+ if (persistence == RELPERSISTENCE_PERMANENT && XLogIsNeeded())
+ {
+ ForkNumber fork;
+
+ for (fork = 0; fork < INIT_FORKNUM ; fork++)
+ {
+ if (smgrexists(r->rd_smgr, fork))
+ log_newpage_range(r, fork,
+ 0, smgrnblocks(r->rd_smgr, fork), false);
+ }
+ }
+
+ table_close(r, NoLock);
+ }
+
+ table_close(classRel, NoLock);
+}
+
/*
* ATRewriteTables: ALTER TABLE phase 3
*/
@@ -5037,45 +5202,52 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
tab->relid,
tab->rewrite);
- /*
- * Create transient table that will receive the modified data.
- *
- * Ensure it is marked correctly as logged or unlogged. We have
- * to do this here so that buffers for the new relfilenode will
- * have the right persistence set, and at the same time ensure
- * that the original filenode's buffers will get read in with the
- * correct setting (i.e. the original one). Otherwise a rollback
- * after the rewrite would possibly result with buffers for the
- * original filenode having the wrong persistence setting.
- *
- * NB: This relies on swap_relation_files() also swapping the
- * persistence. That wouldn't work for pg_class, but that can't be
- * unlogged anyway.
- */
- OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
- lockmode);
+ if (tab->rewrite == AT_REWRITE_ALTER_PERSISTENCE)
+ RelationChangePersistence(tab, persistence, lockmode);
+ else
+ {
+ /*
+ * Create transient table that will receive the modified data.
+ *
+ * Ensure it is marked correctly as logged or unlogged. We
+ * have to do this here so that buffers for the new relfilenode
+ * will have the right persistence set, and at the same time
+ * ensure that the original filenode's buffers will get read in
+ * with the correct setting (i.e. the original one). Otherwise
+ * a rollback after the rewrite would possibly result with
+ * buffers for the original filenode having the wrong
+ * persistence setting.
+ *
+ * NB: This relies on swap_relation_files() also swapping the
+ * persistence. That wouldn't work for pg_class, but that can't
+ * be unlogged anyway.
+ */
+ OIDNewHeap = make_new_heap(tab->relid, NewTableSpace, persistence,
+ lockmode);
- /*
- * Copy the heap data into the new table with the desired
- * modifications, and test the current data within the table
- * against new constraints generated by ALTER TABLE commands.
- */
- ATRewriteTable(tab, OIDNewHeap, lockmode);
+ /*
+ * Copy the heap data into the new table with the desired
+ * modifications, and test the current data within the table
+ * against new constraints generated by ALTER TABLE commands.
+ */
+ ATRewriteTable(tab, OIDNewHeap, lockmode);
- /*
- * Swap the physical files of the old and new heaps, then rebuild
- * indexes and discard the old heap. We can use RecentXmin for
- * the table's new relfrozenxid because we rewrote all the tuples
- * in ATRewriteTable, so no older Xid remains in the table. Also,
- * we never try to swap toast tables by content, since we have no
- * interest in letting this code work on system catalogs.
- */
- finish_heap_swap(tab->relid, OIDNewHeap,
- false, false, true,
- !OidIsValid(tab->newTableSpace),
- RecentXmin,
- ReadNextMultiXactId(),
- persistence);
+ /*
+ * Swap the physical files of the old and new heaps, then
+ * rebuild indexes and discard the old heap. We can use
+ * RecentXmin for the table's new relfrozenxid because we
+ * rewrote all the tuples in ATRewriteTable, so no older Xid
+ * remains in the table. Also, we never try to swap toast
+ * tables by content, since we have no interest in letting this
+ * code work on system catalogs.
+ */
+ finish_heap_swap(tab->relid, OIDNewHeap,
+ false, false, true,
+ !OidIsValid(tab->newTableSpace),
+ RecentXmin,
+ ReadNextMultiXactId(),
+ persistence);
+ }
}
else
{
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 561c212092..eacbdc6447 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -37,6 +37,7 @@
#include "access/xlog.h"
#include "catalog/catalog.h"
#include "catalog/storage.h"
+#include "catalog/storage_xlog.h"
#include "executor/instrument.h"
#include "lib/binaryheap.h"
#include "miscadmin.h"
@@ -3094,6 +3095,93 @@ DropRelFileNodeBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum,
}
}
+/* ---------------------------------------------------------------------
+ * SetRelFileNodeBuffersPersistence
+ *
+ * This function changes the persistence of all buffer pages of a relation
+ * then writes all dirty pages of the relation out to disk when switching
+ * to PERMANENT. (or more accurately, out to kernel disk buffers),
+ * ensuring that the kernel has an up-to-date view of the relation.
+ *
+ * Generally, the caller should be holding AccessExclusiveLock on the
+ * target relation to ensure that no other backend is busy dirtying
+ * more blocks of the relation; the effects can't be expected to last
+ * after the lock is released.
+ *
+ * XXX currently it sequentially searches the buffer pool, should be
+ * changed to more clever ways of searching. This routine is not
+ * used in any performance-critical code paths, so it's not worth
+ * adding additional overhead to normal paths to make it go faster;
+ * but see also DropRelFileNodeBuffers.
+ * --------------------------------------------------------------------
+ */
+void
+SetRelationBuffersPersistence(SMgrRelation srel, bool permanent, bool isRedo)
+{
+ int i;
+ RelFileNodeBackend rnode = srel->smgr_rnode;
+
+ Assert (!RelFileNodeBackendIsTemp(rnode));
+
+ if (!isRedo)
+ log_smgrbufpersistence(&srel->smgr_rnode.node, permanent);
+
+ ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
+
+ for (i = 0; i < NBuffers; i++)
+ {
+ BufferDesc *bufHdr = GetBufferDescriptor(i);
+ uint32 buf_state;
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ continue;
+
+ ReservePrivateRefCountEntry();
+
+ buf_state = LockBufHdr(bufHdr);
+
+ if (!RelFileNodeEquals(bufHdr->tag.rnode, rnode.node))
+ {
+ UnlockBufHdr(bufHdr, buf_state);
+ continue;
+ }
+
+ if (permanent)
+ {
+ /* Init fork is being dropped, drop buffers for it. */
+ if (bufHdr->tag.forkNum == INIT_FORKNUM)
+ {
+ InvalidateBuffer(bufHdr);
+ continue;
+ }
+
+ buf_state |= BM_PERMANENT;
+ pg_atomic_write_u32(&bufHdr->state, buf_state);
+
+ /* we flush this buffer when switching to PERMANENT */
+ if ((buf_state & (BM_VALID | BM_DIRTY)) == (BM_VALID | BM_DIRTY))
+ {
+ PinBuffer_Locked(bufHdr);
+ LWLockAcquire(BufferDescriptorGetContentLock(bufHdr),
+ LW_SHARED);
+ FlushBuffer(bufHdr, srel);
+ LWLockRelease(BufferDescriptorGetContentLock(bufHdr));
+ UnpinBuffer(bufHdr, true);
+ }
+ else
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ else
+ {
+ /* init fork is always BM_PERMANENT. See BufferAlloc */
+ if (bufHdr->tag.forkNum != INIT_FORKNUM)
+ buf_state &= ~BM_PERMANENT;
+
+ UnlockBufHdr(bufHdr, buf_state);
+ }
+ }
+}
+
/* ---------------------------------------------------------------------
* DropRelFileNodesAllBuffers
*
diff --git a/src/backend/storage/file/reinit.c b/src/backend/storage/file/reinit.c
index 40c758d789..0eac1956cc 100644
--- a/src/backend/storage/file/reinit.c
+++ b/src/backend/storage/file/reinit.c
@@ -16,29 +16,50 @@
#include <unistd.h>
+#include "access/xlog.h"
+#include "catalog/pg_tablespace_d.h"
#include "common/relpath.h"
+#include "storage/bufmgr.h"
#include "storage/copydir.h"
#include "storage/fd.h"
+#include "storage/md.h"
#include "storage/reinit.h"
+#include "storage/smgr.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
static void ResetUnloggedRelationsInTablespaceDir(const char *tsdirname,
- int op);
+ Oid tspid, int op);
static void ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname,
- int op);
+ Oid tspid, Oid dbid, int op);
typedef struct
{
Oid reloid; /* hash key */
-} unlogged_relation_entry;
+ bool has_init; /* has INIT fork */
+ bool dirty_init; /* needs to remove INIT fork */
+ bool dirty_all; /* needs to remove all forks */
+} relfile_entry;
/*
- * Reset unlogged relations from before the last restart.
+ * Clean up and reset relation files from before the last restart.
*
- * If op includes UNLOGGED_RELATION_CLEANUP, we remove all forks of any
- * relation with an "init" fork, except for the "init" fork itself.
+ * If op includes UNLOGGED_RELATION_CLEANUP, we perform different operations
+ * depending on the existence of the "cleanup" forks.
*
+ * If CLEANUP_FORKNUM (clup) is present, we remove the init fork of the same
+ * relation along with the clup fork.
+ *
+ * If CLEANUP2_FORKNUM (cln2) is present we remove the whole relation along
+ * with the cln2 fork.
+ *
+ * Otherwise, if the "init" fork is found. we remove all forks of any relation
+ * with the "init" fork, except for the "init" fork itself.
+ *
+ *
+ * If op includes UNLOGGED_RELATION_DROP_BUFFER, we drop all buffers for all
+ * relations that have the "cleanup" and/or the "init" forks.
+ *
* If op includes UNLOGGED_RELATION_INIT, we copy the "init" fork to the main
* fork.
*/
@@ -68,7 +89,7 @@ ResetUnloggedRelations(int op)
/*
* First process unlogged files in pg_default ($PGDATA/base)
*/
- ResetUnloggedRelationsInTablespaceDir("base", op);
+ ResetUnloggedRelationsInTablespaceDir("base", DEFAULTTABLESPACE_OID, op);
/*
* Cycle through directories for all non-default tablespaces.
@@ -77,13 +98,19 @@ ResetUnloggedRelations(int op)
while ((spc_de = ReadDir(spc_dir, "pg_tblspc")) != NULL)
{
+ Oid tspid;
+
if (strcmp(spc_de->d_name, ".") == 0 ||
strcmp(spc_de->d_name, "..") == 0)
continue;
snprintf(temp_path, sizeof(temp_path), "pg_tblspc/%s/%s",
spc_de->d_name, TABLESPACE_VERSION_DIRECTORY);
- ResetUnloggedRelationsInTablespaceDir(temp_path, op);
+
+ tspid = atooid(spc_de->d_name);
+ Assert(tspid != 0);
+
+ ResetUnloggedRelationsInTablespaceDir(temp_path, tspid, op);
}
FreeDir(spc_dir);
@@ -99,7 +126,8 @@ ResetUnloggedRelations(int op)
* Process one tablespace directory for ResetUnloggedRelations
*/
static void
-ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
+ResetUnloggedRelationsInTablespaceDir(const char *tsdirname,
+ Oid tspid, int op)
{
DIR *ts_dir;
struct dirent *de;
@@ -126,6 +154,8 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
while ((de = ReadDir(ts_dir, tsdirname)) != NULL)
{
+ Oid dbid;
+
/*
* We're only interested in the per-database directories, which have
* numeric names. Note that this code will also (properly) ignore "."
@@ -136,7 +166,10 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
snprintf(dbspace_path, sizeof(dbspace_path), "%s/%s",
tsdirname, de->d_name);
- ResetUnloggedRelationsInDbspaceDir(dbspace_path, op);
+ dbid = atooid(de->d_name);
+ Assert(dbid != 0);
+
+ ResetUnloggedRelationsInDbspaceDir(dbspace_path, tspid, dbid, op);
}
FreeDir(ts_dir);
@@ -146,125 +179,226 @@ ResetUnloggedRelationsInTablespaceDir(const char *tsdirname, int op)
* Process one per-dbspace directory for ResetUnloggedRelations
*/
static void
-ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname, int op)
+ResetUnloggedRelationsInDbspaceDir(const char *dbspacedirname,
+ Oid tspid, Oid dbid, int op)
{
DIR *dbspace_dir;
struct dirent *de;
char rm_path[MAXPGPATH * 2];
+ HTAB *hash;
+ HASHCTL ctl;
/* Caller must specify at least one operation. */
- Assert((op & (UNLOGGED_RELATION_CLEANUP | UNLOGGED_RELATION_INIT)) != 0);
+ Assert((op & (UNLOGGED_RELATION_CLEANUP |
+ UNLOGGED_RELATION_DROP_BUFFER |
+ UNLOGGED_RELATION_INIT)) != 0);
/*
* Cleanup is a two-pass operation. First, we go through and identify all
* the files with init forks. Then, we go through again and nuke
* everything with the same OID except the init fork.
*/
+
+ /*
+ * It's possible that someone could create a ton of unlogged relations
+ * in the same database & tablespace, so we'd better use a hash table
+ * rather than an array or linked list to keep track of which files
+ * need to be reset. Otherwise, this cleanup operation would be
+ * O(n^2).
+ */
+ memset(&ctl, 0, sizeof(ctl));
+ ctl.keysize = sizeof(Oid);
+ ctl.entrysize = sizeof(relfile_entry);
+ hash = hash_create("relfilenode cleanup hash",
+ 32, &ctl, HASH_ELEM | HASH_BLOBS);
+
+ /* Collect INIT and CLEANUP forks in the directory. */
+ dbspace_dir = AllocateDir(dbspacedirname);
+ while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
+ {
+ int oidchars;
+ ForkNumber forkNum;
+
+ /* Skip anything that doesn't look like a relation data file. */
+ if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
+ &forkNum))
+ continue;
+
+ if (forkNum == INIT_FORKNUM ||
+ forkNum == CLEANUP_FORKNUM || forkNum == CLEANUP2_FORKNUM)
+ {
+ Oid key;
+ relfile_entry *ent;
+ bool found;
+
+ /*
+ * Record the relfilenode information. If it has the CLEANUP fork,
+ * the relfilenode is in dirty state, where clean up is needed.
+ */
+ key = atooid(de->d_name);
+ ent = hash_search(hash, &key, HASH_ENTER, &found);
+
+ if (!found)
+ {
+ ent->has_init = false;
+ ent->dirty_init = false;
+ ent->dirty_all = false;
+ }
+
+ if (forkNum == CLEANUP_FORKNUM)
+ ent->dirty_init = true;
+ else if (forkNum == CLEANUP2_FORKNUM)
+ ent->dirty_all = true;
+ else
+ {
+ Assert(forkNum == INIT_FORKNUM);
+ ent->has_init = true;
+ }
+ }
+ }
+
+ /* Done with the first pass. */
+ FreeDir(dbspace_dir);
+
+ /* nothing to do if we don't have init nor cleanup forks */
+ if (hash_get_num_entries(hash) < 1)
+ {
+ hash_destroy(hash);
+ return;
+ }
+
+ if ((op & UNLOGGED_RELATION_DROP_BUFFER) != 0)
+ {
+ /*
+ * When we come here after recovery, smgr object for this file might
+ * have been created. In that case we need to drop all buffers then the
+ * smgr object before initializing the unlogged relation. This is safe
+ * as far as no other backends have accessed the relation before
+ * starting archive recovery.
+ */
+ HASH_SEQ_STATUS status;
+ relfile_entry *ent;
+ SMgrRelation *srels = palloc(sizeof(SMgrRelation) * 8);
+ int maxrels = 8;
+ int nrels = 0;
+ int i;
+
+ Assert(!HotStandbyActive());
+
+ hash_seq_init(&status, hash);
+ while((ent = (relfile_entry *) hash_seq_search(&status)) != NULL)
+ {
+ RelFileNodeBackend rel;
+
+ /*
+ * The relation is persistent and stays remain persistent. Don't
+ * drop the buffers for this relation.
+ */
+ if (ent->has_init && ent->dirty_init)
+ continue;
+
+ if (maxrels <= nrels)
+ {
+ maxrels *= 2;
+ srels = repalloc(srels, sizeof(SMgrRelation) * maxrels);
+ }
+
+ rel.backend = InvalidBackendId;
+ rel.node.spcNode = tspid;
+ rel.node.dbNode = dbid;
+ rel.node.relNode = ent->reloid;
+
+ srels[nrels++] = smgropen(rel.node, InvalidBackendId);
+ }
+
+ DropRelFileNodesAllBuffers(srels, nrels);
+
+ for (i = 0 ; i < nrels ; i++)
+ smgrclose(srels[i]);
+ }
+
+ /*
+ * Now, make a second pass and remove anything that matches.
+ */
if ((op & UNLOGGED_RELATION_CLEANUP) != 0)
{
- HTAB *hash;
- HASHCTL ctl;
-
- /*
- * It's possible that someone could create a ton of unlogged relations
- * in the same database & tablespace, so we'd better use a hash table
- * rather than an array or linked list to keep track of which files
- * need to be reset. Otherwise, this cleanup operation would be
- * O(n^2).
- */
- ctl.keysize = sizeof(Oid);
- ctl.entrysize = sizeof(unlogged_relation_entry);
- ctl.hcxt = CurrentMemoryContext;
- hash = hash_create("unlogged relation OIDs", 32, &ctl,
- HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
-
- /* Scan the directory. */
- dbspace_dir = AllocateDir(dbspacedirname);
- while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
- {
- ForkNumber forkNum;
- int oidchars;
- unlogged_relation_entry ent;
-
- /* Skip anything that doesn't look like a relation data file. */
- if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
- &forkNum))
- continue;
-
- /* Also skip it unless this is the init fork. */
- if (forkNum != INIT_FORKNUM)
- continue;
-
- /*
- * Put the OID portion of the name into the hash table, if it
- * isn't already.
- */
- ent.reloid = atooid(de->d_name);
- (void) hash_search(hash, &ent, HASH_ENTER, NULL);
- }
-
- /* Done with the first pass. */
- FreeDir(dbspace_dir);
-
- /*
- * If we didn't find any init forks, there's no point in continuing;
- * we can bail out now.
- */
- if (hash_get_num_entries(hash) == 0)
- {
- hash_destroy(hash);
- return;
- }
-
- /*
- * Now, make a second pass and remove anything that matches.
- */
dbspace_dir = AllocateDir(dbspacedirname);
while ((de = ReadDir(dbspace_dir, dbspacedirname)) != NULL)
{
- ForkNumber forkNum;
- int oidchars;
- unlogged_relation_entry ent;
+ ForkNumber forkNum;
+ int oidchars;
+ Oid key;
+ relfile_entry *ent;
+ RelFileNodeBackend rel;
/* Skip anything that doesn't look like a relation data file. */
if (!parse_filename_for_nontemp_relation(de->d_name, &oidchars,
&forkNum))
continue;
- /* We never remove the init fork. */
- if (forkNum == INIT_FORKNUM)
- continue;
-
/*
* See whether the OID portion of the name shows up in the hash
* table. If so, nuke it!
*/
- ent.reloid = atooid(de->d_name);
- if (hash_search(hash, &ent, HASH_FIND, NULL))
+ key = atooid(de->d_name);
+ ent = hash_search(hash, &key, HASH_FIND, NULL);
+
+ if (!ent)
+ continue;
+
+ if (!ent->dirty_all)
{
- snprintf(rm_path, sizeof(rm_path), "%s/%s",
- dbspacedirname, de->d_name);
- if (unlink(rm_path) < 0)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m",
- rm_path)));
+ /* clean permanent relations don't need cleanup */
+ if (!ent->has_init)
+ continue;
+
+ if (ent->dirty_init)
+ {
+ /*
+ * The crashed trasaction did SET UNLOGGED. This relation
+ * is restored to a LOGGED relation.
+ */
+ if (forkNum != INIT_FORKNUM && forkNum != CLEANUP_FORKNUM)
+ continue;
+ }
else
- elog(DEBUG2, "unlinked file \"%s\"", rm_path);
+ {
+ /*
+ * we don't remove the INIT fork of a non-dirty
+ * relfilenode
+ */
+ if (forkNum == INIT_FORKNUM)
+ continue;
+ }
}
+
+ /* so, nuke it! */
+ snprintf(rm_path, sizeof(rm_path), "%s/%s",
+ dbspacedirname, de->d_name);
+ if (unlink(rm_path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m",
+ rm_path)));
+
+ rel.backend = InvalidBackendId;
+ rel.node.spcNode = tspid;
+ rel.node.dbNode = dbid;
+ rel.node.relNode = atooid(de->d_name);
+
+ ForgetRelationForkSyncRequests(rel, forkNum);
}
/* Cleanup is complete. */
FreeDir(dbspace_dir);
- hash_destroy(hash);
}
+ hash_destroy(hash);
+ hash = NULL;
+
/*
* Initialization happens after cleanup is complete: we copy each init
- * fork file to the corresponding main fork file. Note that if we are
- * asked to do both cleanup and init, we may never get here: if the
- * cleanup code determines that there are no init forks in this dbspace,
- * it will return before we get to this point.
+ * fork file to the corresponding main fork file.
*/
if ((op & UNLOGGED_RELATION_INIT) != 0)
{
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 0643d714fb..6b37195c52 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -338,8 +338,10 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
if (ret == 0 || errno != ENOENT)
{
ret = unlink(path);
+
+ /* failure of removing cleanup fork leads to a data loss. */
if (ret < 0 && errno != ENOENT)
- ereport(WARNING,
+ ereport((forkNum != CLEANUP_FORKNUM ? WARNING : ERROR),
(errcode_for_file_access(),
errmsg("could not remove file \"%s\": %m", path)));
}
@@ -1024,6 +1026,15 @@ register_forget_request(RelFileNodeBackend rnode, ForkNumber forknum,
RegisterSyncRequest(&tag, SYNC_FORGET_REQUEST, true /* retryOnError */ );
}
+/*
+ * ForgetRelationForkSyncRequests -- forget any fsyncs and unlinks for a fork
+ */
+void
+ForgetRelationForkSyncRequests(RelFileNodeBackend rnode, ForkNumber forknum)
+{
+ register_forget_request(rnode, forknum, 0);
+}
+
/*
* ForgetDatabaseSyncRequests -- forget any fsyncs and unlinks for a DB
*/
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 4dc24649df..96480e321d 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -662,6 +662,12 @@ smgrimmedsync(SMgrRelation reln, ForkNumber forknum)
smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum);
}
+void
+smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo)
+{
+ smgrsw[reln->smgr_which].smgr_unlink(reln->smgr_rnode, forknum, isRedo);
+}
+
/*
* AtEOXact_SMgr
*
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 1f5c426ec0..479dcc248e 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -34,7 +34,9 @@ const char *const forkNames[] = {
"main", /* MAIN_FORKNUM */
"fsm", /* FSM_FORKNUM */
"vm", /* VISIBILITYMAP_FORKNUM */
- "init" /* INIT_FORKNUM */
+ "init", /* INIT_FORKNUM */
+ "clup", /* CLEANUP_FORKNUM */
+ "cln2" /* CLEANUP2_FORKNUM */
};
StaticAssertDecl(lengthof(forkNames) == (MAX_FORKNUM + 1),
diff --git a/src/include/catalog/storage.h b/src/include/catalog/storage.h
index 0ab32b44e9..382623159c 100644
--- a/src/include/catalog/storage.h
+++ b/src/include/catalog/storage.h
@@ -23,6 +23,8 @@
extern int wal_skip_threshold;
extern SMgrRelation RelationCreateStorage(RelFileNode rnode, char relpersistence);
+extern void RelationCreateInitFork(Relation rel);
+extern void RelationDropInitFork(Relation rel);
extern void RelationDropStorage(Relation rel);
extern void RelationPreserveStorage(RelFileNode rnode, bool atCommit);
extern void RelationPreTruncate(Relation rel);
diff --git a/src/include/catalog/storage_xlog.h b/src/include/catalog/storage_xlog.h
index f0814f1458..0fd0832a8b 100644
--- a/src/include/catalog/storage_xlog.h
+++ b/src/include/catalog/storage_xlog.h
@@ -22,13 +22,17 @@
/*
* Declarations for smgr-related XLOG records
*
- * Note: we log file creation and truncation here, but logging of deletion
- * actions is handled by xact.c, because it is part of transaction commit.
+ * Note: we log file creation, truncation, deletion and persistence change
+ * here. logging of deletion actions is mainly handled by xact.c, because it is
+ * part of transaction commit, but we log deletions happens outside of a
+ * transaction.
*/
/* XLOG gives us high 4 bits */
#define XLOG_SMGR_CREATE 0x10
#define XLOG_SMGR_TRUNCATE 0x20
+#define XLOG_SMGR_UNLINK 0x30
+#define XLOG_SMGR_BUFPERSISTENCE 0x40
typedef struct xl_smgr_create
{
@@ -36,6 +40,18 @@ typedef struct xl_smgr_create
ForkNumber forkNum;
} xl_smgr_create;
+typedef struct xl_smgr_unlink
+{
+ RelFileNode rnode;
+ ForkNumber forkNum;
+} xl_smgr_unlink;
+
+typedef struct xl_smgr_bufpersistence
+{
+ RelFileNode rnode;
+ bool persistence;
+} xl_smgr_bufpersistence;
+
/* flags for xl_smgr_truncate */
#define SMGR_TRUNCATE_HEAP 0x0001
#define SMGR_TRUNCATE_VM 0x0002
@@ -51,6 +67,8 @@ typedef struct xl_smgr_truncate
} xl_smgr_truncate;
extern void log_smgrcreate(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrunlink(const RelFileNode *rnode, ForkNumber forkNum);
+extern void log_smgrbufpersistence(const RelFileNode *rnode, bool persistence);
extern void smgr_redo(XLogReaderState *record);
extern void smgr_desc(StringInfo buf, XLogReaderState *record);
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index a44be11ca0..040070aa2b 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -43,7 +43,9 @@ typedef enum ForkNumber
MAIN_FORKNUM = 0,
FSM_FORKNUM,
VISIBILITYMAP_FORKNUM,
- INIT_FORKNUM
+ INIT_FORKNUM,
+ CLEANUP_FORKNUM,
+ CLEANUP2_FORKNUM
/*
* NOTE: if you add a new fork, change MAX_FORKNUM and possibly
@@ -52,7 +54,7 @@ typedef enum ForkNumber
*/
} ForkNumber;
-#define MAX_FORKNUM INIT_FORKNUM
+#define MAX_FORKNUM CLEANUP2_FORKNUM
#define FORKNAMECHARS 4 /* max chars for a fork name */
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index fb00fda6a7..ccb0a388f6 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -205,6 +205,8 @@ extern void FlushRelationsAllBuffers(struct SMgrRelationData **smgrs, int nrels)
extern void FlushDatabaseBuffers(Oid dbid);
extern void DropRelFileNodeBuffers(struct SMgrRelationData *smgr_reln, ForkNumber *forkNum,
int nforks, BlockNumber *firstDelBlock);
+extern void SetRelationBuffersPersistence(struct SMgrRelationData *srel,
+ bool permanent, bool isRedo);
extern void DropRelFileNodesAllBuffers(struct SMgrRelationData **smgr_reln, int nnodes);
extern void DropDatabaseBuffers(Oid dbid);
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index 752b440864..3cbbbf2edd 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -41,6 +41,8 @@ extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
+extern void ForgetRelationForkSyncRequests(RelFileNodeBackend rnode,
+ ForkNumber forknum);
extern void ForgetDatabaseSyncRequests(Oid dbid);
extern void DropRelationFiles(RelFileNode *delrels, int ndelrels, bool isRedo);
diff --git a/src/include/storage/reinit.h b/src/include/storage/reinit.h
index fad1e5c473..b969ba8e86 100644
--- a/src/include/storage/reinit.h
+++ b/src/include/storage/reinit.h
@@ -23,6 +23,7 @@ extern bool parse_filename_for_nontemp_relation(const char *name,
int *oidchars, ForkNumber *fork);
#define UNLOGGED_RELATION_CLEANUP 0x0001
-#define UNLOGGED_RELATION_INIT 0x0002
+#define UNLOGGED_RELATION_DROP_BUFFER 0x0002
+#define UNLOGGED_RELATION_INIT 0x0004
#endif /* REINIT_H */
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index a6fbf7b6a6..1ac3e4a74a 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -86,6 +86,7 @@ extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode);
extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
+extern void smgrunlink(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
--
2.27.0
----Next_Part(Thu_Jan_14_17_32_17_2021_289)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v5-0002-New-command-ALTER-TABLE-ALL-IN-TABLESPACE-SET-LOG.patch"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
@ 2025-02-13 16:50 Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Dolgov @ 2025-02-13 16:50 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Thu, Feb 13, 2025 at 01:47:01PM GMT, Álvaro Herrera wrote:
> Also, how wed are you to
> "query_id_merge_values" as a name? It's not in any way obvious that
> this is about values in arrays. How about query_id_squash_arrays? Or
> are you thinking in having values in other types of structures squashed
> as well, and that this first patch does it for arrays only but you want
> the GUC to also control some future feature?
>
> (I think I prefer "squash" here as a verb to "merge").
Yeah, when choosing the name I was trying to keep it a bit generic. The
high level goal is to reduce repeated non-essential parts, and arrays of
constants are one clear scenario, but there could be more to it. Having
said that I don't have any particular plans for extending this logic so
far. I've ended up with query_id_squash_values, how does this sound?
> I think calling func_volatile potentially once per array element is not
> good; this might cause dozens/thousands of identical syscache lookups.
> Maybe we can pass an initially NIL list from IsMergeableConstList (as
> List **), which IsMergeableConst fills with OIDs of functions that have
> been checked and found acceptable. Then the second time around we
> search the list first and only do func_volatile() after not finding a
> match.
Good point, added.
> Another thing I didn't quite understand is why you did this rather
> baroque-looking list scan:
I'm pretty sure there was some reason behind it, but when you pointed it
out that reason has promptly vanished in a puff of confusion. Fixed.
From 212f5534fb0f99e5daa74ea4464231faec157a58 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <[email protected]>
Date: Tue, 3 Dec 2024 14:55:45 +0100
Subject: [PATCH v24] Prevent jumbling of every element in ArrayExpr
pg_stat_statements produces multiple entries for queries like
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
depending on the number of parameters, because every element of
ArrayExpr is jumbled. In certain situations it's undesirable, especially
if the list becomes too large.
Make an array of Const expressions contribute only the first/last
elements to the jumble hash. Allow to enable this behavior via the new
pg_stat_statements parameter query_id_squash_values with the default value off.
Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane,
Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier, Sutou Kouhei,
Sami Imseih
Tested-by: Chengxi Sun, Yasuo Honda
---
contrib/pg_stat_statements/Makefile | 2 +-
.../pg_stat_statements/expected/merging.out | 432 ++++++++++++++++++
contrib/pg_stat_statements/meson.build | 1 +
.../pg_stat_statements/pg_stat_statements.c | 47 +-
contrib/pg_stat_statements/sql/merging.sql | 169 +++++++
doc/src/sgml/config.sgml | 27 ++
doc/src/sgml/pgstatstatements.sgml | 28 +-
src/backend/nodes/gen_node_support.pl | 21 +-
src/backend/nodes/queryjumblefuncs.c | 165 ++++++-
src/backend/postmaster/launch_backend.c | 3 +
src/backend/utils/misc/guc_tables.c | 10 +
src/backend/utils/misc/postgresql.conf.sample | 2 +-
src/include/nodes/nodes.h | 3 +
src/include/nodes/primnodes.h | 2 +-
src/include/nodes/queryjumble.h | 8 +-
15 files changed, 894 insertions(+), 26 deletions(-)
create mode 100644 contrib/pg_stat_statements/expected/merging.out
create mode 100644 contrib/pg_stat_statements/sql/merging.sql
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 241c02587b..eef8d69cc4 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -20,7 +20,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = select dml cursors utility level_tracking planning \
user_activity wal entry_timestamp privileges extended \
- parallel cleanup oldextversions
+ parallel cleanup oldextversions merging
# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out
new file mode 100644
index 0000000000..881174d0ca
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/merging.out
@@ -0,0 +1,432 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-------------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(4 rows)
+
+-- Normal scenario, too many simple constants for an IN query
+SET query_id_squash_values = on;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1) | 1
+ SELECT * FROM test_merge WHERE id IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1) | 1
+ SELECT * FROM test_merge WHERE id IN (...) | 4
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(4 rows)
+
+-- More conditions in the query
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN (...) and data = $3 | 3
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- No constants simplification for OpExpr
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+-- In the following two queries the operator expressions (+) and (@) have
+-- different oppno, and will be given different query_id if merged, even though
+-- the normalized query will be the same
+SELECT * FROM test_merge WHERE id IN
+ (1 + 1, 2 + 2, 3 + 3, 4 + 4, 5 + 5, 6 + 6, 7 + 7, 8 + 8, 9 + 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN
+ (@ '-1', @ '-2', @ '-3', @ '-4', @ '-5', @ '-6', @ '-7', @ '-8', @ '-9');
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN +| 1
+ ($1 + $2, $3 + $4, $5 + $6, $7 + $8, $9 + $10, $11 + $12, $13 + $14, $15 + $16, $17 + $18) |
+ SELECT * FROM test_merge WHERE id IN +| 1
+ (@ $1, @ $2, @ $3, @ $4, @ $5, @ $6, @ $7, @ $8, @ $9) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(3 rows)
+
+-- FuncExpr
+-- Verify multiple type representation end up with the same query_id
+CREATE TABLE test_float (data float);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT data FROM test_float WHERE data IN (1, 2);
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN (1, '2');
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN ('1', 2);
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN ('1', '2');
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN (1.0, 1.0);
+ data
+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT data FROM test_float WHERE data IN (...) | 5
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Numeric type, implicit cast is merged
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_numeric WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------+-------
+ SELECT * FROM test_merge_numeric WHERE data IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, implicit cast is merged
+CREATE TABLE test_merge_bigint (id int, data bigint);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-----------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE data IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, explicit cast is not merged
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE data IN
+ (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+ 7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE data IN +| 1
+ ($1::bigint, $2::bigint, $3::bigint, $4::bigint, $5::bigint, $6::bigint,+|
+ $7::bigint, $8::bigint, $9::bigint, $10::bigint, $11::bigint) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, long tokens with parenthesis
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE id IN
+ (abs(100), abs(200), abs(300), abs(400), abs(500), abs(600), abs(700),
+ abs(800), abs(900), abs(1000), ((abs(1100))));
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE id IN +| 1
+ (abs($1), abs($2), abs($3), abs($4), abs($5), abs($6), abs($7),+|
+ abs($8), abs($9), abs($10), ((abs($11)))) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- CoerceViaIO, SubLink instead of a Const
+CREATE TABLE test_merge_jsonb (id int, data jsonb);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_jsonb WHERE data IN
+ ((SELECT '"1"')::jsonb, (SELECT '"2"')::jsonb, (SELECT '"3"')::jsonb,
+ (SELECT '"4"')::jsonb, (SELECT '"5"')::jsonb, (SELECT '"6"')::jsonb,
+ (SELECT '"7"')::jsonb, (SELECT '"8"')::jsonb, (SELECT '"9"')::jsonb,
+ (SELECT '"10"')::jsonb);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_jsonb WHERE data IN +| 1
+ ((SELECT $1)::jsonb, (SELECT $2)::jsonb, (SELECT $3)::jsonb,+|
+ (SELECT $4)::jsonb, (SELECT $5)::jsonb, (SELECT $6)::jsonb,+|
+ (SELECT $7)::jsonb, (SELECT $8)::jsonb, (SELECT $9)::jsonb,+|
+ (SELECT $10)::jsonb) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- CoerceViaIO
+-- Create some dummy type to force CoerceViaIO
+CREATE TYPE casttesttype;
+CREATE FUNCTION casttesttype_in(cstring)
+ RETURNS casttesttype
+ AS 'textin'
+ LANGUAGE internal STRICT IMMUTABLE;
+NOTICE: return type casttesttype is only a shell
+CREATE FUNCTION casttesttype_out(casttesttype)
+ RETURNS cstring
+ AS 'textout'
+ LANGUAGE internal STRICT IMMUTABLE;
+NOTICE: argument type casttesttype is only a shell
+LINE 1: CREATE FUNCTION casttesttype_out(casttesttype)
+ ^
+CREATE TYPE casttesttype (
+ internallength = variable,
+ input = casttesttype_in,
+ output = casttesttype_out,
+ alignment = int4
+);
+CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE FUNCTION casttesttype_eq(casttesttype, casttesttype)
+returns boolean language sql immutable as $$
+ SELECT true
+$$;
+CREATE OPERATOR = (
+ leftarg = casttesttype,
+ rightarg = casttesttype,
+ procedure = casttesttype_eq,
+ commutator = =);
+CREATE TABLE test_merge_cast (id int, data casttesttype);
+-- Use the introduced type to construct a list of CoerceViaIO around Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_cast WHERE data IN
+ (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+ 4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+ 7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+ 10::int4::casttesttype, 11::int4::casttesttype);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge_cast WHERE data IN +| 1
+ (...::int4::casttesttype) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Some casting expression are simplified to Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_jsonb WHERE data IN
+ (('"1"')::jsonb, ('"2"')::jsonb, ('"3"')::jsonb, ('"4"')::jsonb,
+ ( '"5"')::jsonb, ( '"6"')::jsonb, ( '"7"')::jsonb, ( '"8"')::jsonb,
+ ( '"9"')::jsonb, ( '"10"')::jsonb);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge_jsonb WHERE data IN +| 1
+ ((...)::jsonb) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- RelabelType
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN (...::oid) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Test constants evaluation in a CTE, which was causing issues in the past
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+ result
+--------
+(0 rows)
+
+-- Simple array would be merged as well
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT ARRAY[1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+ array
+------------------------
+ {1,2,3,4,5,6,7,8,9,10}
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT ARRAY[...] | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+RESET query_id_squash_values;
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 4446af58c5..8a96aff625 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -56,6 +56,7 @@ tests += {
'parallel',
'cleanup',
'oldextversions',
+ 'merging',
],
'regress_args': ['--temp-config', files('pg_stat_statements.conf')],
# Disabled because these tests require
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index bebf8134eb..1aa5021367 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -295,7 +295,6 @@ static bool pgss_track_planning = false; /* whether to track planning
* duration */
static bool pgss_save = true; /* whether to save stats across shutdown */
-
#define pgss_enabled(level) \
(!IsParallelWorker() && \
(pgss_track == PGSS_TRACK_ALL || \
@@ -2809,6 +2808,10 @@ generate_normalized_query(JumbleState *jstate, const char *query,
n_quer_loc = 0, /* Normalized query byte location */
last_off = 0, /* Offset from start for previous tok */
last_tok_len = 0; /* Length (in bytes) of that tok */
+ bool merged_interval = false; /* Currently processed constants
+ belong to a merged constants
+ interval. */
+
/*
* Get constants' lengths (core system only gives us locations). Note
@@ -2832,7 +2835,6 @@ generate_normalized_query(JumbleState *jstate, const char *query,
{
int off, /* Offset from start for cur tok */
tok_len; /* Length (in bytes) of that tok */
-
off = jstate->clocations[i].location;
/* Adjust recorded location if we're dealing with partial string */
off -= query_loc;
@@ -2847,13 +2849,44 @@ generate_normalized_query(JumbleState *jstate, const char *query,
len_to_wrt -= last_tok_len;
Assert(len_to_wrt >= 0);
- memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
- n_quer_loc += len_to_wrt;
- /* And insert a param symbol in place of the constant token */
- n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
- i + 1 + jstate->highest_extern_param_id);
+ /* Normal path, non merged constant */
+ if (!jstate->clocations[i].merged)
+ {
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* And insert a param symbol in place of the constant token */
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
+ i + 1 + jstate->highest_extern_param_id);
+
+ /* In case previous constants were merged away, stop doing that */
+ merged_interval = false;
+ }
+ else if (!merged_interval)
+ {
+ /*
+ * We are not inside a merged interval yet, which means it is the
+ * the first merged constant.
+ *
+ * A merged constants interval must be represented via two
+ * constants with the merged flag. Currently we are at the first,
+ * verify there is another one.
+ */
+ Assert(i + 1 < jstate->clocations_count);
+ Assert(jstate->clocations[i + 1].merged);
+
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* Remember to skip until a non merged constant appears */
+ merged_interval = true;
+
+ /* Mark the interval in the normalized query */
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "...");
+ }
+ /* Otherwise the constant is merged away, move forward */
quer_loc = off + tok_len;
last_off = off;
last_tok_len = tok_len;
diff --git a/contrib/pg_stat_statements/sql/merging.sql b/contrib/pg_stat_statements/sql/merging.sql
new file mode 100644
index 0000000000..a302d3d91f
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/merging.sql
@@ -0,0 +1,169 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+
+CREATE TABLE test_merge (id int, data int);
+
+-- IN queries
+
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Normal scenario, too many simple constants for an IN query
+SET query_id_squash_values = on;
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- More conditions in the query
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9) and data = 2;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) and data = 2;
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- No constants simplification for OpExpr
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+-- In the following two queries the operator expressions (+) and (@) have
+-- different oppno, and will be given different query_id if merged, even though
+-- the normalized query will be the same
+SELECT * FROM test_merge WHERE id IN
+ (1 + 1, 2 + 2, 3 + 3, 4 + 4, 5 + 5, 6 + 6, 7 + 7, 8 + 8, 9 + 9);
+SELECT * FROM test_merge WHERE id IN
+ (@ '-1', @ '-2', @ '-3', @ '-4', @ '-5', @ '-6', @ '-7', @ '-8', @ '-9');
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- FuncExpr
+
+-- Verify multiple type representation end up with the same query_id
+CREATE TABLE test_float (data float);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT data FROM test_float WHERE data IN (1, 2);
+SELECT data FROM test_float WHERE data IN (1, '2');
+SELECT data FROM test_float WHERE data IN ('1', 2);
+SELECT data FROM test_float WHERE data IN ('1', '2');
+SELECT data FROM test_float WHERE data IN (1.0, 1.0);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Numeric type, implicit cast is merged
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_numeric WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, implicit cast is merged
+CREATE TABLE test_merge_bigint (id int, data bigint);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, explicit cast is not merged
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE data IN
+ (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+ 7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, long tokens with parenthesis
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE id IN
+ (abs(100), abs(200), abs(300), abs(400), abs(500), abs(600), abs(700),
+ abs(800), abs(900), abs(1000), ((abs(1100))));
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- CoerceViaIO, SubLink instead of a Const
+CREATE TABLE test_merge_jsonb (id int, data jsonb);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_jsonb WHERE data IN
+ ((SELECT '"1"')::jsonb, (SELECT '"2"')::jsonb, (SELECT '"3"')::jsonb,
+ (SELECT '"4"')::jsonb, (SELECT '"5"')::jsonb, (SELECT '"6"')::jsonb,
+ (SELECT '"7"')::jsonb, (SELECT '"8"')::jsonb, (SELECT '"9"')::jsonb,
+ (SELECT '"10"')::jsonb);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- CoerceViaIO
+
+-- Create some dummy type to force CoerceViaIO
+CREATE TYPE casttesttype;
+
+CREATE FUNCTION casttesttype_in(cstring)
+ RETURNS casttesttype
+ AS 'textin'
+ LANGUAGE internal STRICT IMMUTABLE;
+
+CREATE FUNCTION casttesttype_out(casttesttype)
+ RETURNS cstring
+ AS 'textout'
+ LANGUAGE internal STRICT IMMUTABLE;
+
+CREATE TYPE casttesttype (
+ internallength = variable,
+ input = casttesttype_in,
+ output = casttesttype_out,
+ alignment = int4
+);
+
+CREATE CAST (int4 AS casttesttype) WITH INOUT;
+
+CREATE FUNCTION casttesttype_eq(casttesttype, casttesttype)
+returns boolean language sql immutable as $$
+ SELECT true
+$$;
+
+CREATE OPERATOR = (
+ leftarg = casttesttype,
+ rightarg = casttesttype,
+ procedure = casttesttype_eq,
+ commutator = =);
+
+CREATE TABLE test_merge_cast (id int, data casttesttype);
+
+-- Use the introduced type to construct a list of CoerceViaIO around Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_cast WHERE data IN
+ (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+ 4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+ 7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+ 10::int4::casttesttype, 11::int4::casttesttype);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Some casting expression are simplified to Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_jsonb WHERE data IN
+ (('"1"')::jsonb, ('"2"')::jsonb, ('"3"')::jsonb, ('"4"')::jsonb,
+ ( '"5"')::jsonb, ( '"6"')::jsonb, ( '"7"')::jsonb, ( '"8"')::jsonb,
+ ( '"9"')::jsonb, ( '"10"')::jsonb);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- RelabelType
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Test constants evaluation in a CTE, which was causing issues in the past
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+
+-- Simple array would be merged as well
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT ARRAY[1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+RESET query_id_squash_values;
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5e4f201e09..ddc8c853a2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8436,6 +8436,33 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-query-id-squash-values" xreflabel="query_id_squash_values">
+ <term><varname>query_id_squash_values</varname> (<type>bool</type>)
+ <indexterm>
+ <primary><varname>query_id_squash_values</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies how an array of constants (e.g. for an <literal>IN</literal>
+ clause) contributes to the query identifier computation. Normally every
+ element of an array contributes to the query identifier, which means the
+ same query will get multiple different identifiers, one for each
+ occurrence with an array of different lenght.
+
+ If this parameter is on, an array of constants will contribute only the
+ first and the last elements to the query identifier. It means two
+ occurences of the same query, where the only difference is number of
+ constants in the array, are going to get the same query identifier.
+ Such queries are represented in form <literal>'(...)'</literal>.
+
+ The parameter could be used to reduce amount of repeating data stored
+ via <xref linkend="pgstatstatements"/>. The default value is
+ <literal>off</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-statement-stats">
<term><varname>log_statement_stats</varname> (<type>boolean</type>)
<indexterm>
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 501b468e9a..9ce4988008 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -621,11 +621,28 @@
<para>
In some cases, queries with visibly different texts might get merged into a
- single <structname>pg_stat_statements</structname> entry. Normally this will happen
- only for semantically equivalent queries, but there is a small chance of
- hash collisions causing unrelated queries to be merged into one entry.
- (This cannot happen for queries belonging to different users or databases,
- however.)
+ single <structname>pg_stat_statements</structname> entry. Normally this
+ will happen only for semantically equivalent queries, or if
+ <varname>query_id_squash_values</varname> is enabled and the only difference
+ between queries is the length of an array with constants they contain:
+
+<screen>
+=# SET query_id_squash_values = on;
+=# SELECT pg_stat_statements_reset();
+=# SELECT * FROM test WHERE a IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+=# SELECT * FROM test WHERE a IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12);
+=# SELECT query, calls FROM pg_stat_statements;
+-[ RECORD 1 ]------------------------------
+query | SELECT * FROM test WHERE a IN (...)
+calls | 2
+-[ RECORD 2 ]------------------------------
+query | SELECT pg_stat_statements_reset()
+calls | 1
+</screen>
+
+ But there is a small chance of hash collisions causing unrelated queries to
+ be merged into one entry. (This cannot happen for queries belonging to
+ different users or databases, however.)
</para>
<para>
@@ -956,6 +973,7 @@
</para>
</listitem>
</varlistentry>
+
</variablelist>
<para>
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 1a657f7e0a..c421664879 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -476,6 +476,7 @@ foreach my $infile (@ARGV)
equal_ignore_if_zero
query_jumble_ignore
query_jumble_location
+ query_jumble_merge
read_write_ignore
write_only_relids
write_only_nondefault_pathtarget
@@ -1283,6 +1284,7 @@ _jumble${n}(JumbleState *jstate, Node *node)
my @a = @{ $node_type_info{$n}->{field_attrs}{$f} };
my $query_jumble_ignore = $struct_no_query_jumble;
my $query_jumble_location = 0;
+ my $query_jumble_merge = 0;
# extract per-field attributes
foreach my $a (@a)
@@ -1295,21 +1297,34 @@ _jumble${n}(JumbleState *jstate, Node *node)
{
$query_jumble_location = 1;
}
+ elsif ($a eq 'query_jumble_merge')
+ {
+ $query_jumble_merge = 1;
+ }
}
# node type
if (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/)
and elem $1, @node_types)
{
- print $jff "\tJUMBLE_NODE($f);\n"
- unless $query_jumble_ignore;
+ # Merge constants if requested.
+ if ($query_jumble_merge)
+ {
+ print $jff "\tJUMBLE_ELEMENTS($f);\n"
+ unless $query_jumble_ignore;
+ }
+ else
+ {
+ print $jff "\tJUMBLE_NODE($f);\n"
+ unless $query_jumble_ignore;
+ }
}
elsif ($t eq 'ParseLoc')
{
# Track the node's location only if directly requested.
if ($query_jumble_location)
{
- print $jff "\tJUMBLE_LOCATION($f);\n"
+ print $jff "\tJUMBLE_LOCATION($f, false);\n"
unless $query_jumble_ignore;
}
}
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index b103a28193..b7f13dc658 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -32,9 +32,13 @@
*/
#include "postgres.h"
+#include "access/transam.h"
+#include "catalog/pg_proc.h"
#include "common/hashfn.h"
#include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
#include "nodes/queryjumble.h"
+#include "utils/lsyscache.h"
#include "parser/scansup.h"
#define JUMBLE_SIZE 1024 /* query serialization buffer size */
@@ -42,6 +46,9 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
+/* Whether to merge constants in a list when computing query_id */
+bool query_id_squash_values = false;
+
/*
* True when compute_query_id is ON or AUTO, and a module requests them.
*
@@ -53,8 +60,10 @@ bool query_id_enabled = false;
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
-static void RecordConstLocation(JumbleState *jstate, int location);
+static void RecordConstLocation(JumbleState *jstate,
+ int location, bool merged);
static void _jumbleNode(JumbleState *jstate, Node *node);
+static void _jumbleElements(JumbleState *jstate, List *elements);
static void _jumbleA_Const(JumbleState *jstate, Node *node);
static void _jumbleList(JumbleState *jstate, Node *node);
static void _jumbleVariableSetStmt(JumbleState *jstate, Node *node);
@@ -198,11 +207,15 @@ AppendJumble(JumbleState *jstate, const unsigned char *item, Size size)
}
/*
- * Record location of constant within query string of query tree
- * that is currently being walked.
+ * Record location of constant within query string of query tree that is
+ * currently being walked.
+ *
+ * Merged argument signals that the constant represents the first or the last
+ * element in a series of merged constants, and everything but the first/last
+ * element contributes nothing to the jumble hash.
*/
static void
-RecordConstLocation(JumbleState *jstate, int location)
+RecordConstLocation(JumbleState *jstate, int location, bool merged)
{
/* -1 indicates unknown or undefined location */
if (location >= 0)
@@ -218,15 +231,126 @@ RecordConstLocation(JumbleState *jstate, int location)
}
jstate->clocations[jstate->clocations_count].location = location;
/* initialize lengths to -1 to simplify third-party module usage */
+ jstate->clocations[jstate->clocations_count].merged = merged;
jstate->clocations[jstate->clocations_count].length = -1;
jstate->clocations_count++;
}
}
+/*
+ * Verify few simple cases where we can deduce that the expression is a
+ * constant:
+ *
+ * - Simplify the expression, if it's wrapped into RelabelType and CoerceViaIO.
+ * - If it's a FuncExpr, check if the function is an immutable builtin
+ * function doing implicit cast with constant arguments.
+ * - Otherwise test if the expression is a simple Const.
+ *
+ * We could also handle some simple OpExpr here as well, but since such queries
+ * will also have opno jumbled, this might lead to a confusing situation where
+ * two different queries end up with the same normalized query but different
+ * query_id.
+ *
+ * The argument known_immutable_funcs contains known function OIDs that were
+ * already proven to be immutable. If the expression to verify is a FuncExpr,
+ * we first check this list, and only if not found, test the function
+ * volatility and store the result back. Since most of the time constants
+ * merging will be dealing with same type of expressions, this avoids
+ * performing func_volatile over and over for the same functions.
+ *
+ * Note that we intentionally do not recurse on the function arguments and only
+ * test them for being Const expression for simplicity.
+ */
+static bool
+IsMergeableConst(Node *element, List **known_immutable_funcs)
+{
+ if (IsA(element, RelabelType))
+ element = (Node *) ((RelabelType *) element)->arg;
+
+ if (IsA(element, CoerceViaIO))
+ element = (Node *) ((CoerceViaIO *) element)->arg;
+
+ if(IsA(element, FuncExpr))
+ {
+ FuncExpr *func = (FuncExpr *) element;
+ ListCell *temp;
+
+ if (func->funcid > FirstGenbkiObjectId)
+ return false;
+
+ if (func->funcformat != COERCE_IMPLICIT_CAST)
+ return false;
+
+ if (!list_member_oid(*known_immutable_funcs, func->funcid))
+ {
+ /* Not found in the cache, verify and add if needed */
+ if(func_volatile(func->funcid) != PROVOLATILE_IMMUTABLE)
+ return false;
+
+ *known_immutable_funcs = lappend_oid(*known_immutable_funcs,
+ func->funcid);
+ }
+
+ foreach(temp, func->args)
+ {
+ Node *arg = lfirst(temp);
+
+ if (!IsA(arg, Const))
+ return false;
+ }
+
+ return true;
+ }
+
+ if (!IsA(element, Const))
+ return false;
+
+ return true;
+}
+
+/*
+ * Verify if the provided list could be merged down, which means it contains
+ * only constant expressions.
+ *
+ * Return value indicates if merging is possible.
+ *
+ * Note that this function searches only for explicit Const nodes and does not
+ * try to simplify expressions.
+ */
+static bool
+IsMergeableConstList(List *elements, Node **firstExpr, Node **lastExpr)
+{
+ ListCell *temp;
+
+ /* To keep track of immutable functions in elements */
+ List *immutable_funcs = NIL;
+
+ if (elements == NIL)
+ return false;
+
+ if (!query_id_squash_values)
+ {
+ /* Merging is disabled, process everything one by one */
+ return false;
+ }
+
+ foreach(temp, elements)
+ {
+ if (!IsMergeableConst(lfirst(temp), &immutable_funcs))
+ return false;
+ }
+ *firstExpr = linitial(elements);
+ *lastExpr = llast(elements);
+
+ return true;
+}
+
#define JUMBLE_NODE(item) \
_jumbleNode(jstate, (Node *) expr->item)
-#define JUMBLE_LOCATION(location) \
- RecordConstLocation(jstate, expr->location)
+#define JUMBLE_ELEMENTS(list) \
+ _jumbleElements(jstate, (List *) expr->list)
+#define JUMBLE_LOCATION(location, merged) \
+ RecordConstLocation(jstate, expr->location, merged)
#define JUMBLE_FIELD(item) \
AppendJumble(jstate, (const unsigned char *) &(expr->item), sizeof(expr->item))
#define JUMBLE_FIELD_SINGLE(item) \
@@ -239,6 +363,33 @@ do { \
#include "queryjumblefuncs.funcs.c"
+static void
+_jumbleElements(JumbleState *jstate, List *elements)
+{
+ Node *first, *last;
+ if (IsMergeableConstList(elements, &first, &last))
+ {
+ /*
+ * Both first and last constants have to be recorded. The first one
+ * will indicate the merged interval, the last one will tell us the
+ * length of the interval within the query text.
+ *
+ * Note that for the last exression we actually need not the expression
+ * location (which is the leftmost expression), but where it ends. For
+ * the limited set of supported cases now (implicit coerce via
+ * FuncExpr, Const) it's fine to use exprLocation, but if more complex
+ * composite expressions will be supported, e.g. OpExpr or FuncExpr as
+ * an explicit call, the rightmost expression will be needed.
+ */
+ RecordConstLocation(jstate, exprLocation(first), true);
+ RecordConstLocation(jstate, exprLocation(last), true);
+ }
+ else
+ {
+ _jumbleNode(jstate, (Node *) elements);
+ }
+}
+
static void
_jumbleNode(JumbleState *jstate, Node *node)
{
@@ -375,5 +526,5 @@ _jumbleVariableSetStmt(JumbleState *jstate, Node *node)
if (expr->jumble_args)
JUMBLE_NODE(args);
JUMBLE_FIELD(is_local);
- JUMBLE_LOCATION(location);
+ JUMBLE_LOCATION(location, false);
}
diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
index a97a1eda6d..052b700390 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -115,6 +115,7 @@ typedef struct
bool redirection_done;
bool IsBinaryUpgrade;
bool query_id_enabled;
+ bool query_id_squash_values;
int max_safe_fds;
int MaxBackends;
int num_pmchild_slots;
@@ -744,6 +745,7 @@ save_backend_variables(BackendParameters *param,
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
param->query_id_enabled = query_id_enabled;
+ param->query_id_squash_values = query_id_squash_values;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -1004,6 +1006,7 @@ restore_backend_variables(BackendParameters *param)
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
query_id_enabled = param->query_id_enabled;
+ query_id_squash_values = param->query_id_squash_values;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 226af43fe2..eca30ba1a8 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2106,6 +2106,16 @@ struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"query_id_squash_values", PGC_USERSET, STATS_MONITORING,
+ gettext_noop("Allows to merge constants in a list when computing "
+ "query_id."),
+ },
+ &query_id_squash_values,
+ false,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d472987ed4..15939394bc 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -645,12 +645,12 @@
# - Monitoring -
#compute_query_id = auto
+#query_id_squash_values = off
#log_statement_stats = off
#log_parser_stats = off
#log_planner_stats = off
#log_executor_stats = off
-
#------------------------------------------------------------------------------
# VACUUMING
#------------------------------------------------------------------------------
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 580238bfab..5656302544 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -108,6 +108,9 @@ typedef enum NodeTag
* - query_jumble_location: Mark the field as a location to track. This is
* only allowed for integer fields that include "location" in their name.
*
+ * - query_jumble_merge: Allow to merge the field values for the query
+ * jumbling.
+ *
* - read_as(VALUE): In nodeRead(), replace the field's value with VALUE.
*
* - read_write_ignore: Ignore the field for read/write. This is only allowed
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 839e71d52f..89587d4c10 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1394,7 +1394,7 @@ typedef struct ArrayExpr
/* common type of array elements */
Oid element_typeid pg_node_attr(query_jumble_ignore);
/* the array elements or sub-arrays */
- List *elements;
+ List *elements pg_node_attr(query_jumble_merge);
/* true if elements are sub-arrays */
bool multidims pg_node_attr(query_jumble_ignore);
/* token location, or -1 if unknown */
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index 50eb956658..d2f1c1e310 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -23,6 +23,12 @@ typedef struct LocationLen
{
int location; /* start offset in query text */
int length; /* length in bytes, or -1 to ignore */
+
+ /*
+ * Indicates the constant represents the beginning or the end of a merged
+ * constants interval.
+ */
+ bool merged;
} LocationLen;
/*
@@ -62,12 +68,12 @@ enum ComputeQueryIdType
/* GUC parameters */
extern PGDLLIMPORT int compute_query_id;
-
extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query);
extern void EnableQueryId(void);
extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_squash_values;
/*
* Returns whether query identifier computation has been enabled, either
base-commit: 773c51dd39ada5f107a3656377a9611ff89132f1
--
2.45.1
Attachments:
[text/plain] v24-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch (44.6K, ../../z7daminuuasjjunvhul5q5zwvjjrkvkldizq6oriufxnjvq6l5@6tgsib66vp7c/2-v24-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch)
download | inline diff:
From 212f5534fb0f99e5daa74ea4464231faec157a58 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <[email protected]>
Date: Tue, 3 Dec 2024 14:55:45 +0100
Subject: [PATCH v24] Prevent jumbling of every element in ArrayExpr
pg_stat_statements produces multiple entries for queries like
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
depending on the number of parameters, because every element of
ArrayExpr is jumbled. In certain situations it's undesirable, especially
if the list becomes too large.
Make an array of Const expressions contribute only the first/last
elements to the jumble hash. Allow to enable this behavior via the new
pg_stat_statements parameter query_id_squash_values with the default value off.
Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane,
Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier, Sutou Kouhei,
Sami Imseih
Tested-by: Chengxi Sun, Yasuo Honda
---
contrib/pg_stat_statements/Makefile | 2 +-
.../pg_stat_statements/expected/merging.out | 432 ++++++++++++++++++
contrib/pg_stat_statements/meson.build | 1 +
.../pg_stat_statements/pg_stat_statements.c | 47 +-
contrib/pg_stat_statements/sql/merging.sql | 169 +++++++
doc/src/sgml/config.sgml | 27 ++
doc/src/sgml/pgstatstatements.sgml | 28 +-
src/backend/nodes/gen_node_support.pl | 21 +-
src/backend/nodes/queryjumblefuncs.c | 165 ++++++-
src/backend/postmaster/launch_backend.c | 3 +
src/backend/utils/misc/guc_tables.c | 10 +
src/backend/utils/misc/postgresql.conf.sample | 2 +-
src/include/nodes/nodes.h | 3 +
src/include/nodes/primnodes.h | 2 +-
src/include/nodes/queryjumble.h | 8 +-
15 files changed, 894 insertions(+), 26 deletions(-)
create mode 100644 contrib/pg_stat_statements/expected/merging.out
create mode 100644 contrib/pg_stat_statements/sql/merging.sql
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 241c02587b..eef8d69cc4 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -20,7 +20,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = select dml cursors utility level_tracking planning \
user_activity wal entry_timestamp privileges extended \
- parallel cleanup oldextversions
+ parallel cleanup oldextversions merging
# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out
new file mode 100644
index 0000000000..881174d0ca
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/merging.out
@@ -0,0 +1,432 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-------------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(4 rows)
+
+-- Normal scenario, too many simple constants for an IN query
+SET query_id_squash_values = on;
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1) | 1
+ SELECT * FROM test_merge WHERE id IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(3 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN ($1) | 1
+ SELECT * FROM test_merge WHERE id IN (...) | 4
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 1
+(4 rows)
+
+-- More conditions in the query
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) and data = 2;
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN (...) and data = $3 | 3
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- No constants simplification for OpExpr
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+-- In the following two queries the operator expressions (+) and (@) have
+-- different oppno, and will be given different query_id if merged, even though
+-- the normalized query will be the same
+SELECT * FROM test_merge WHERE id IN
+ (1 + 1, 2 + 2, 3 + 3, 4 + 4, 5 + 5, 6 + 6, 7 + 7, 8 + 8, 9 + 9);
+ id | data
+----+------
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN
+ (@ '-1', @ '-2', @ '-3', @ '-4', @ '-5', @ '-6', @ '-7', @ '-8', @ '-9');
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN +| 1
+ ($1 + $2, $3 + $4, $5 + $6, $7 + $8, $9 + $10, $11 + $12, $13 + $14, $15 + $16, $17 + $18) |
+ SELECT * FROM test_merge WHERE id IN +| 1
+ (@ $1, @ $2, @ $3, @ $4, @ $5, @ $6, @ $7, @ $8, @ $9) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(3 rows)
+
+-- FuncExpr
+-- Verify multiple type representation end up with the same query_id
+CREATE TABLE test_float (data float);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT data FROM test_float WHERE data IN (1, 2);
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN (1, '2');
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN ('1', 2);
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN ('1', '2');
+ data
+------
+(0 rows)
+
+SELECT data FROM test_float WHERE data IN (1.0, 1.0);
+ data
+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT data FROM test_float WHERE data IN (...) | 5
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Numeric type, implicit cast is merged
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_numeric WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------+-------
+ SELECT * FROM test_merge_numeric WHERE data IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, implicit cast is merged
+CREATE TABLE test_merge_bigint (id int, data bigint);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-----------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE data IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, explicit cast is not merged
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE data IN
+ (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+ 7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE data IN +| 1
+ ($1::bigint, $2::bigint, $3::bigint, $4::bigint, $5::bigint, $6::bigint,+|
+ $7::bigint, $8::bigint, $9::bigint, $10::bigint, $11::bigint) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Bigint, long tokens with parenthesis
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_bigint WHERE id IN
+ (abs(100), abs(200), abs(300), abs(400), abs(500), abs(600), abs(700),
+ abs(800), abs(900), abs(1000), ((abs(1100))));
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+-------------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_bigint WHERE id IN +| 1
+ (abs($1), abs($2), abs($3), abs($4), abs($5), abs($6), abs($7),+|
+ abs($8), abs($9), abs($10), ((abs($11)))) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- CoerceViaIO, SubLink instead of a Const
+CREATE TABLE test_merge_jsonb (id int, data jsonb);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_jsonb WHERE data IN
+ ((SELECT '"1"')::jsonb, (SELECT '"2"')::jsonb, (SELECT '"3"')::jsonb,
+ (SELECT '"4"')::jsonb, (SELECT '"5"')::jsonb, (SELECT '"6"')::jsonb,
+ (SELECT '"7"')::jsonb, (SELECT '"8"')::jsonb, (SELECT '"9"')::jsonb,
+ (SELECT '"10"')::jsonb);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------------------------+-------
+ SELECT * FROM test_merge_jsonb WHERE data IN +| 1
+ ((SELECT $1)::jsonb, (SELECT $2)::jsonb, (SELECT $3)::jsonb,+|
+ (SELECT $4)::jsonb, (SELECT $5)::jsonb, (SELECT $6)::jsonb,+|
+ (SELECT $7)::jsonb, (SELECT $8)::jsonb, (SELECT $9)::jsonb,+|
+ (SELECT $10)::jsonb) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- CoerceViaIO
+-- Create some dummy type to force CoerceViaIO
+CREATE TYPE casttesttype;
+CREATE FUNCTION casttesttype_in(cstring)
+ RETURNS casttesttype
+ AS 'textin'
+ LANGUAGE internal STRICT IMMUTABLE;
+NOTICE: return type casttesttype is only a shell
+CREATE FUNCTION casttesttype_out(casttesttype)
+ RETURNS cstring
+ AS 'textout'
+ LANGUAGE internal STRICT IMMUTABLE;
+NOTICE: argument type casttesttype is only a shell
+LINE 1: CREATE FUNCTION casttesttype_out(casttesttype)
+ ^
+CREATE TYPE casttesttype (
+ internallength = variable,
+ input = casttesttype_in,
+ output = casttesttype_out,
+ alignment = int4
+);
+CREATE CAST (int4 AS casttesttype) WITH INOUT;
+CREATE FUNCTION casttesttype_eq(casttesttype, casttesttype)
+returns boolean language sql immutable as $$
+ SELECT true
+$$;
+CREATE OPERATOR = (
+ leftarg = casttesttype,
+ rightarg = casttesttype,
+ procedure = casttesttype_eq,
+ commutator = =);
+CREATE TABLE test_merge_cast (id int, data casttesttype);
+-- Use the introduced type to construct a list of CoerceViaIO around Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_cast WHERE data IN
+ (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+ 4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+ 7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+ 10::int4::casttesttype, 11::int4::casttesttype);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge_cast WHERE data IN +| 1
+ (...::int4::casttesttype) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Some casting expression are simplified to Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge_jsonb WHERE data IN
+ (('"1"')::jsonb, ('"2"')::jsonb, ('"3"')::jsonb, ('"4"')::jsonb,
+ ( '"5"')::jsonb, ( '"6"')::jsonb, ( '"7"')::jsonb, ( '"8"')::jsonb,
+ ( '"9"')::jsonb, ( '"10"')::jsonb);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge_jsonb WHERE data IN +| 1
+ ((...)::jsonb) |
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- RelabelType
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+ id | data
+----+------
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT * FROM test_merge WHERE id IN (...::oid) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+-- Test constants evaluation in a CTE, which was causing issues in the past
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+ result
+--------
+(0 rows)
+
+-- Simple array would be merged as well
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t
+---
+ t
+(1 row)
+
+SELECT ARRAY[1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+ array
+------------------------
+ {1,2,3,4,5,6,7,8,9,10}
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+----------------------------------------------------+-------
+ SELECT ARRAY[...] | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
+
+RESET query_id_squash_values;
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 4446af58c5..8a96aff625 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -56,6 +56,7 @@ tests += {
'parallel',
'cleanup',
'oldextversions',
+ 'merging',
],
'regress_args': ['--temp-config', files('pg_stat_statements.conf')],
# Disabled because these tests require
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index bebf8134eb..1aa5021367 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -295,7 +295,6 @@ static bool pgss_track_planning = false; /* whether to track planning
* duration */
static bool pgss_save = true; /* whether to save stats across shutdown */
-
#define pgss_enabled(level) \
(!IsParallelWorker() && \
(pgss_track == PGSS_TRACK_ALL || \
@@ -2809,6 +2808,10 @@ generate_normalized_query(JumbleState *jstate, const char *query,
n_quer_loc = 0, /* Normalized query byte location */
last_off = 0, /* Offset from start for previous tok */
last_tok_len = 0; /* Length (in bytes) of that tok */
+ bool merged_interval = false; /* Currently processed constants
+ belong to a merged constants
+ interval. */
+
/*
* Get constants' lengths (core system only gives us locations). Note
@@ -2832,7 +2835,6 @@ generate_normalized_query(JumbleState *jstate, const char *query,
{
int off, /* Offset from start for cur tok */
tok_len; /* Length (in bytes) of that tok */
-
off = jstate->clocations[i].location;
/* Adjust recorded location if we're dealing with partial string */
off -= query_loc;
@@ -2847,13 +2849,44 @@ generate_normalized_query(JumbleState *jstate, const char *query,
len_to_wrt -= last_tok_len;
Assert(len_to_wrt >= 0);
- memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
- n_quer_loc += len_to_wrt;
- /* And insert a param symbol in place of the constant token */
- n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
- i + 1 + jstate->highest_extern_param_id);
+ /* Normal path, non merged constant */
+ if (!jstate->clocations[i].merged)
+ {
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* And insert a param symbol in place of the constant token */
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
+ i + 1 + jstate->highest_extern_param_id);
+
+ /* In case previous constants were merged away, stop doing that */
+ merged_interval = false;
+ }
+ else if (!merged_interval)
+ {
+ /*
+ * We are not inside a merged interval yet, which means it is the
+ * the first merged constant.
+ *
+ * A merged constants interval must be represented via two
+ * constants with the merged flag. Currently we are at the first,
+ * verify there is another one.
+ */
+ Assert(i + 1 < jstate->clocations_count);
+ Assert(jstate->clocations[i + 1].merged);
+
+ memcpy(norm_query + n_quer_loc, query + quer_loc, len_to_wrt);
+ n_quer_loc += len_to_wrt;
+
+ /* Remember to skip until a non merged constant appears */
+ merged_interval = true;
+
+ /* Mark the interval in the normalized query */
+ n_quer_loc += sprintf(norm_query + n_quer_loc, "...");
+ }
+ /* Otherwise the constant is merged away, move forward */
quer_loc = off + tok_len;
last_off = off;
last_tok_len = tok_len;
diff --git a/contrib/pg_stat_statements/sql/merging.sql b/contrib/pg_stat_statements/sql/merging.sql
new file mode 100644
index 0000000000..a302d3d91f
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/merging.sql
@@ -0,0 +1,169 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+
+CREATE TABLE test_merge (id int, data int);
+
+-- IN queries
+
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Normal scenario, too many simple constants for an IN query
+SET query_id_squash_values = on;
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- More conditions in the query
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9) and data = 2;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10) and data = 2;
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11) and data = 2;
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- No constants simplification for OpExpr
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
+-- In the following two queries the operator expressions (+) and (@) have
+-- different oppno, and will be given different query_id if merged, even though
+-- the normalized query will be the same
+SELECT * FROM test_merge WHERE id IN
+ (1 + 1, 2 + 2, 3 + 3, 4 + 4, 5 + 5, 6 + 6, 7 + 7, 8 + 8, 9 + 9);
+SELECT * FROM test_merge WHERE id IN
+ (@ '-1', @ '-2', @ '-3', @ '-4', @ '-5', @ '-6', @ '-7', @ '-8', @ '-9');
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- FuncExpr
+
+-- Verify multiple type representation end up with the same query_id
+CREATE TABLE test_float (data float);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT data FROM test_float WHERE data IN (1, 2);
+SELECT data FROM test_float WHERE data IN (1, '2');
+SELECT data FROM test_float WHERE data IN ('1', 2);
+SELECT data FROM test_float WHERE data IN ('1', '2');
+SELECT data FROM test_float WHERE data IN (1.0, 1.0);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Numeric type, implicit cast is merged
+CREATE TABLE test_merge_numeric (id int, data numeric(5, 2));
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_numeric WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, implicit cast is merged
+CREATE TABLE test_merge_bigint (id int, data bigint);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE data IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, explicit cast is not merged
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE data IN
+ (1::bigint, 2::bigint, 3::bigint, 4::bigint, 5::bigint, 6::bigint,
+ 7::bigint, 8::bigint, 9::bigint, 10::bigint, 11::bigint);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Bigint, long tokens with parenthesis
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_bigint WHERE id IN
+ (abs(100), abs(200), abs(300), abs(400), abs(500), abs(600), abs(700),
+ abs(800), abs(900), abs(1000), ((abs(1100))));
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- CoerceViaIO, SubLink instead of a Const
+CREATE TABLE test_merge_jsonb (id int, data jsonb);
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_jsonb WHERE data IN
+ ((SELECT '"1"')::jsonb, (SELECT '"2"')::jsonb, (SELECT '"3"')::jsonb,
+ (SELECT '"4"')::jsonb, (SELECT '"5"')::jsonb, (SELECT '"6"')::jsonb,
+ (SELECT '"7"')::jsonb, (SELECT '"8"')::jsonb, (SELECT '"9"')::jsonb,
+ (SELECT '"10"')::jsonb);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- CoerceViaIO
+
+-- Create some dummy type to force CoerceViaIO
+CREATE TYPE casttesttype;
+
+CREATE FUNCTION casttesttype_in(cstring)
+ RETURNS casttesttype
+ AS 'textin'
+ LANGUAGE internal STRICT IMMUTABLE;
+
+CREATE FUNCTION casttesttype_out(casttesttype)
+ RETURNS cstring
+ AS 'textout'
+ LANGUAGE internal STRICT IMMUTABLE;
+
+CREATE TYPE casttesttype (
+ internallength = variable,
+ input = casttesttype_in,
+ output = casttesttype_out,
+ alignment = int4
+);
+
+CREATE CAST (int4 AS casttesttype) WITH INOUT;
+
+CREATE FUNCTION casttesttype_eq(casttesttype, casttesttype)
+returns boolean language sql immutable as $$
+ SELECT true
+$$;
+
+CREATE OPERATOR = (
+ leftarg = casttesttype,
+ rightarg = casttesttype,
+ procedure = casttesttype_eq,
+ commutator = =);
+
+CREATE TABLE test_merge_cast (id int, data casttesttype);
+
+-- Use the introduced type to construct a list of CoerceViaIO around Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_cast WHERE data IN
+ (1::int4::casttesttype, 2::int4::casttesttype, 3::int4::casttesttype,
+ 4::int4::casttesttype, 5::int4::casttesttype, 6::int4::casttesttype,
+ 7::int4::casttesttype, 8::int4::casttesttype, 9::int4::casttesttype,
+ 10::int4::casttesttype, 11::int4::casttesttype);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Some casting expression are simplified to Const
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge_jsonb WHERE data IN
+ (('"1"')::jsonb, ('"2"')::jsonb, ('"3"')::jsonb, ('"4"')::jsonb,
+ ( '"5"')::jsonb, ( '"6"')::jsonb, ( '"7"')::jsonb, ( '"8"')::jsonb,
+ ( '"9"')::jsonb, ( '"10"')::jsonb);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- RelabelType
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT * FROM test_merge WHERE id IN (1::oid, 2::oid, 3::oid, 4::oid, 5::oid, 6::oid, 7::oid, 8::oid, 9::oid);
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+-- Test constants evaluation in a CTE, which was causing issues in the past
+WITH cte AS (
+ SELECT 'const' as const FROM test_merge
+)
+SELECT ARRAY['a', 'b', 'c', const::varchar] AS result
+FROM cte;
+
+-- Simple array would be merged as well
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT ARRAY[1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+RESET query_id_squash_values;
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 5e4f201e09..ddc8c853a2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8436,6 +8436,33 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-query-id-squash-values" xreflabel="query_id_squash_values">
+ <term><varname>query_id_squash_values</varname> (<type>bool</type>)
+ <indexterm>
+ <primary><varname>query_id_squash_values</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Specifies how an array of constants (e.g. for an <literal>IN</literal>
+ clause) contributes to the query identifier computation. Normally every
+ element of an array contributes to the query identifier, which means the
+ same query will get multiple different identifiers, one for each
+ occurrence with an array of different lenght.
+
+ If this parameter is on, an array of constants will contribute only the
+ first and the last elements to the query identifier. It means two
+ occurences of the same query, where the only difference is number of
+ constants in the array, are going to get the same query identifier.
+ Such queries are represented in form <literal>'(...)'</literal>.
+
+ The parameter could be used to reduce amount of repeating data stored
+ via <xref linkend="pgstatstatements"/>. The default value is
+ <literal>off</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-statement-stats">
<term><varname>log_statement_stats</varname> (<type>boolean</type>)
<indexterm>
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 501b468e9a..9ce4988008 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -621,11 +621,28 @@
<para>
In some cases, queries with visibly different texts might get merged into a
- single <structname>pg_stat_statements</structname> entry. Normally this will happen
- only for semantically equivalent queries, but there is a small chance of
- hash collisions causing unrelated queries to be merged into one entry.
- (This cannot happen for queries belonging to different users or databases,
- however.)
+ single <structname>pg_stat_statements</structname> entry. Normally this
+ will happen only for semantically equivalent queries, or if
+ <varname>query_id_squash_values</varname> is enabled and the only difference
+ between queries is the length of an array with constants they contain:
+
+<screen>
+=# SET query_id_squash_values = on;
+=# SELECT pg_stat_statements_reset();
+=# SELECT * FROM test WHERE a IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+=# SELECT * FROM test WHERE a IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12);
+=# SELECT query, calls FROM pg_stat_statements;
+-[ RECORD 1 ]------------------------------
+query | SELECT * FROM test WHERE a IN (...)
+calls | 2
+-[ RECORD 2 ]------------------------------
+query | SELECT pg_stat_statements_reset()
+calls | 1
+</screen>
+
+ But there is a small chance of hash collisions causing unrelated queries to
+ be merged into one entry. (This cannot happen for queries belonging to
+ different users or databases, however.)
</para>
<para>
@@ -956,6 +973,7 @@
</para>
</listitem>
</varlistentry>
+
</variablelist>
<para>
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 1a657f7e0a..c421664879 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -476,6 +476,7 @@ foreach my $infile (@ARGV)
equal_ignore_if_zero
query_jumble_ignore
query_jumble_location
+ query_jumble_merge
read_write_ignore
write_only_relids
write_only_nondefault_pathtarget
@@ -1283,6 +1284,7 @@ _jumble${n}(JumbleState *jstate, Node *node)
my @a = @{ $node_type_info{$n}->{field_attrs}{$f} };
my $query_jumble_ignore = $struct_no_query_jumble;
my $query_jumble_location = 0;
+ my $query_jumble_merge = 0;
# extract per-field attributes
foreach my $a (@a)
@@ -1295,21 +1297,34 @@ _jumble${n}(JumbleState *jstate, Node *node)
{
$query_jumble_location = 1;
}
+ elsif ($a eq 'query_jumble_merge')
+ {
+ $query_jumble_merge = 1;
+ }
}
# node type
if (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/)
and elem $1, @node_types)
{
- print $jff "\tJUMBLE_NODE($f);\n"
- unless $query_jumble_ignore;
+ # Merge constants if requested.
+ if ($query_jumble_merge)
+ {
+ print $jff "\tJUMBLE_ELEMENTS($f);\n"
+ unless $query_jumble_ignore;
+ }
+ else
+ {
+ print $jff "\tJUMBLE_NODE($f);\n"
+ unless $query_jumble_ignore;
+ }
}
elsif ($t eq 'ParseLoc')
{
# Track the node's location only if directly requested.
if ($query_jumble_location)
{
- print $jff "\tJUMBLE_LOCATION($f);\n"
+ print $jff "\tJUMBLE_LOCATION($f, false);\n"
unless $query_jumble_ignore;
}
}
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index b103a28193..b7f13dc658 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -32,9 +32,13 @@
*/
#include "postgres.h"
+#include "access/transam.h"
+#include "catalog/pg_proc.h"
#include "common/hashfn.h"
#include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
#include "nodes/queryjumble.h"
+#include "utils/lsyscache.h"
#include "parser/scansup.h"
#define JUMBLE_SIZE 1024 /* query serialization buffer size */
@@ -42,6 +46,9 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
+/* Whether to merge constants in a list when computing query_id */
+bool query_id_squash_values = false;
+
/*
* True when compute_query_id is ON or AUTO, and a module requests them.
*
@@ -53,8 +60,10 @@ bool query_id_enabled = false;
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
-static void RecordConstLocation(JumbleState *jstate, int location);
+static void RecordConstLocation(JumbleState *jstate,
+ int location, bool merged);
static void _jumbleNode(JumbleState *jstate, Node *node);
+static void _jumbleElements(JumbleState *jstate, List *elements);
static void _jumbleA_Const(JumbleState *jstate, Node *node);
static void _jumbleList(JumbleState *jstate, Node *node);
static void _jumbleVariableSetStmt(JumbleState *jstate, Node *node);
@@ -198,11 +207,15 @@ AppendJumble(JumbleState *jstate, const unsigned char *item, Size size)
}
/*
- * Record location of constant within query string of query tree
- * that is currently being walked.
+ * Record location of constant within query string of query tree that is
+ * currently being walked.
+ *
+ * Merged argument signals that the constant represents the first or the last
+ * element in a series of merged constants, and everything but the first/last
+ * element contributes nothing to the jumble hash.
*/
static void
-RecordConstLocation(JumbleState *jstate, int location)
+RecordConstLocation(JumbleState *jstate, int location, bool merged)
{
/* -1 indicates unknown or undefined location */
if (location >= 0)
@@ -218,15 +231,126 @@ RecordConstLocation(JumbleState *jstate, int location)
}
jstate->clocations[jstate->clocations_count].location = location;
/* initialize lengths to -1 to simplify third-party module usage */
+ jstate->clocations[jstate->clocations_count].merged = merged;
jstate->clocations[jstate->clocations_count].length = -1;
jstate->clocations_count++;
}
}
+/*
+ * Verify few simple cases where we can deduce that the expression is a
+ * constant:
+ *
+ * - Simplify the expression, if it's wrapped into RelabelType and CoerceViaIO.
+ * - If it's a FuncExpr, check if the function is an immutable builtin
+ * function doing implicit cast with constant arguments.
+ * - Otherwise test if the expression is a simple Const.
+ *
+ * We could also handle some simple OpExpr here as well, but since such queries
+ * will also have opno jumbled, this might lead to a confusing situation where
+ * two different queries end up with the same normalized query but different
+ * query_id.
+ *
+ * The argument known_immutable_funcs contains known function OIDs that were
+ * already proven to be immutable. If the expression to verify is a FuncExpr,
+ * we first check this list, and only if not found, test the function
+ * volatility and store the result back. Since most of the time constants
+ * merging will be dealing with same type of expressions, this avoids
+ * performing func_volatile over and over for the same functions.
+ *
+ * Note that we intentionally do not recurse on the function arguments and only
+ * test them for being Const expression for simplicity.
+ */
+static bool
+IsMergeableConst(Node *element, List **known_immutable_funcs)
+{
+ if (IsA(element, RelabelType))
+ element = (Node *) ((RelabelType *) element)->arg;
+
+ if (IsA(element, CoerceViaIO))
+ element = (Node *) ((CoerceViaIO *) element)->arg;
+
+ if(IsA(element, FuncExpr))
+ {
+ FuncExpr *func = (FuncExpr *) element;
+ ListCell *temp;
+
+ if (func->funcid > FirstGenbkiObjectId)
+ return false;
+
+ if (func->funcformat != COERCE_IMPLICIT_CAST)
+ return false;
+
+ if (!list_member_oid(*known_immutable_funcs, func->funcid))
+ {
+ /* Not found in the cache, verify and add if needed */
+ if(func_volatile(func->funcid) != PROVOLATILE_IMMUTABLE)
+ return false;
+
+ *known_immutable_funcs = lappend_oid(*known_immutable_funcs,
+ func->funcid);
+ }
+
+ foreach(temp, func->args)
+ {
+ Node *arg = lfirst(temp);
+
+ if (!IsA(arg, Const))
+ return false;
+ }
+
+ return true;
+ }
+
+ if (!IsA(element, Const))
+ return false;
+
+ return true;
+}
+
+/*
+ * Verify if the provided list could be merged down, which means it contains
+ * only constant expressions.
+ *
+ * Return value indicates if merging is possible.
+ *
+ * Note that this function searches only for explicit Const nodes and does not
+ * try to simplify expressions.
+ */
+static bool
+IsMergeableConstList(List *elements, Node **firstExpr, Node **lastExpr)
+{
+ ListCell *temp;
+
+ /* To keep track of immutable functions in elements */
+ List *immutable_funcs = NIL;
+
+ if (elements == NIL)
+ return false;
+
+ if (!query_id_squash_values)
+ {
+ /* Merging is disabled, process everything one by one */
+ return false;
+ }
+
+ foreach(temp, elements)
+ {
+ if (!IsMergeableConst(lfirst(temp), &immutable_funcs))
+ return false;
+ }
+ *firstExpr = linitial(elements);
+ *lastExpr = llast(elements);
+
+ return true;
+}
+
#define JUMBLE_NODE(item) \
_jumbleNode(jstate, (Node *) expr->item)
-#define JUMBLE_LOCATION(location) \
- RecordConstLocation(jstate, expr->location)
+#define JUMBLE_ELEMENTS(list) \
+ _jumbleElements(jstate, (List *) expr->list)
+#define JUMBLE_LOCATION(location, merged) \
+ RecordConstLocation(jstate, expr->location, merged)
#define JUMBLE_FIELD(item) \
AppendJumble(jstate, (const unsigned char *) &(expr->item), sizeof(expr->item))
#define JUMBLE_FIELD_SINGLE(item) \
@@ -239,6 +363,33 @@ do { \
#include "queryjumblefuncs.funcs.c"
+static void
+_jumbleElements(JumbleState *jstate, List *elements)
+{
+ Node *first, *last;
+ if (IsMergeableConstList(elements, &first, &last))
+ {
+ /*
+ * Both first and last constants have to be recorded. The first one
+ * will indicate the merged interval, the last one will tell us the
+ * length of the interval within the query text.
+ *
+ * Note that for the last exression we actually need not the expression
+ * location (which is the leftmost expression), but where it ends. For
+ * the limited set of supported cases now (implicit coerce via
+ * FuncExpr, Const) it's fine to use exprLocation, but if more complex
+ * composite expressions will be supported, e.g. OpExpr or FuncExpr as
+ * an explicit call, the rightmost expression will be needed.
+ */
+ RecordConstLocation(jstate, exprLocation(first), true);
+ RecordConstLocation(jstate, exprLocation(last), true);
+ }
+ else
+ {
+ _jumbleNode(jstate, (Node *) elements);
+ }
+}
+
static void
_jumbleNode(JumbleState *jstate, Node *node)
{
@@ -375,5 +526,5 @@ _jumbleVariableSetStmt(JumbleState *jstate, Node *node)
if (expr->jumble_args)
JUMBLE_NODE(args);
JUMBLE_FIELD(is_local);
- JUMBLE_LOCATION(location);
+ JUMBLE_LOCATION(location, false);
}
diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
index a97a1eda6d..052b700390 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -115,6 +115,7 @@ typedef struct
bool redirection_done;
bool IsBinaryUpgrade;
bool query_id_enabled;
+ bool query_id_squash_values;
int max_safe_fds;
int MaxBackends;
int num_pmchild_slots;
@@ -744,6 +745,7 @@ save_backend_variables(BackendParameters *param,
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
param->query_id_enabled = query_id_enabled;
+ param->query_id_squash_values = query_id_squash_values;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -1004,6 +1006,7 @@ restore_backend_variables(BackendParameters *param)
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
query_id_enabled = param->query_id_enabled;
+ query_id_squash_values = param->query_id_squash_values;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 226af43fe2..eca30ba1a8 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2106,6 +2106,16 @@ struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"query_id_squash_values", PGC_USERSET, STATS_MONITORING,
+ gettext_noop("Allows to merge constants in a list when computing "
+ "query_id."),
+ },
+ &query_id_squash_values,
+ false,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index d472987ed4..15939394bc 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -645,12 +645,12 @@
# - Monitoring -
#compute_query_id = auto
+#query_id_squash_values = off
#log_statement_stats = off
#log_parser_stats = off
#log_planner_stats = off
#log_executor_stats = off
-
#------------------------------------------------------------------------------
# VACUUMING
#------------------------------------------------------------------------------
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 580238bfab..5656302544 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -108,6 +108,9 @@ typedef enum NodeTag
* - query_jumble_location: Mark the field as a location to track. This is
* only allowed for integer fields that include "location" in their name.
*
+ * - query_jumble_merge: Allow to merge the field values for the query
+ * jumbling.
+ *
* - read_as(VALUE): In nodeRead(), replace the field's value with VALUE.
*
* - read_write_ignore: Ignore the field for read/write. This is only allowed
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 839e71d52f..89587d4c10 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1394,7 +1394,7 @@ typedef struct ArrayExpr
/* common type of array elements */
Oid element_typeid pg_node_attr(query_jumble_ignore);
/* the array elements or sub-arrays */
- List *elements;
+ List *elements pg_node_attr(query_jumble_merge);
/* true if elements are sub-arrays */
bool multidims pg_node_attr(query_jumble_ignore);
/* token location, or -1 if unknown */
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index 50eb956658..d2f1c1e310 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -23,6 +23,12 @@ typedef struct LocationLen
{
int location; /* start offset in query text */
int length; /* length in bytes, or -1 to ignore */
+
+ /*
+ * Indicates the constant represents the beginning or the end of a merged
+ * constants interval.
+ */
+ bool merged;
} LocationLen;
/*
@@ -62,12 +68,12 @@ enum ComputeQueryIdType
/* GUC parameters */
extern PGDLLIMPORT int compute_query_id;
-
extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query);
extern void EnableQueryId(void);
extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_squash_values;
/*
* Returns whether query identifier computation has been enabled, either
base-commit: 773c51dd39ada5f107a3656377a9611ff89132f1
--
2.45.1
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-02-13 23:08 ` Sami Imseih <[email protected]>
2025-02-14 08:36 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Sami Imseih @ 2025-02-13 23:08 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
Hi,
Thanks for the updated patch!
I spent some time looking at v24 today, and I have some findings/comments.
1/
Constants passed as parameters to a prepared statement will not be
handled as expected. I did not not test explicit PREPARE/EXECUTE statement,
but I assume it will have the same issue.
postgres=# show query_id_squash_values;
query_id_squash_values
------------------------
on
(1 row)
postgres=# select from foo where col_bigint in ($1, $2, $3, $4) \bind 1 2 3 4
postgres-# ;
--
(0 rows)
postgres=# select from foo where col_bigint in ($1, $2, $3, $4, $5)
\bind 1 2 3 4 5
postgres-# ;
--
(0 rows)
postgres=# select query, queryid, calls from pg_stat_statements where
query like 'select%from foo where%' order by stats_since asc;
query |
queryid | calls
----------------------------------------------------------+----------------------+-------
select from foo where col_bigint in ($1, $2, $3, $4) |
-1169585827903667511 | 1
select from foo where col_bigint in ($1, $2, $3, $4, $5) |
-5591703027615838766 | 1
(2 rows)
I think the answer is here is to also check for "Param" when deciding
if an element
should be merged.
i.e.
if (!IsA(element, Const) && !IsA(element, Param))
2/
This case with an array passed to aa function seems to cause a regression
in pg_stat_statements query text. As you can see the text is incomplete.
CREATE OR REPLACE FUNCTION arrtest(i int[]) RETURNS void AS $$
BEGIN
NULL;
END;
$$ LANGUAGE plpgsql;
postgres=# select arrtest(array[1, 2]) from foo where col_bigint in (1, 2, 3);
arrtest
---------
(0 rows)
postgres=# select query from pg_stat_statements;
query
---------------------------
select arrtest(array[...)
(1 row)
it should otherwise look like this:
postgres=# select query from pg_stat_statements;
query
-------------------------------------------------------------------------
select arrtest(array[$1, $2]) from foo where col_bigint in ($3, $4, $5)
(1 row)
3/
A typo in the docs.
c/lenght/length
+ occurrence with an array of different lenght.
4/
+ <para>
+ Specifies how an array of constants (e.g. for an <literal>IN</literal>
+ clause) contributes to the query identifier computation.
Is this parameter specific to only useful to merge the values of an IN list.
Should the documentation be more specific and say that only IN lists
will benefit from this parameter?
Also, if there is only 1 value in the list, it will have a different
queryId than
that of the same query in which more than 1 value is passed to the IN list.
Should the documentation be clear about that?
5/
pg_node_attr of query_jumble_merge is doing something
very specific to the elements list of an ArrayExpr. The
merge code likely cannot be used for other node types.
/* the array elements or sub-arrays */
- List *elements;
+ List *elements pg_node_attr(query_jumble_merge);
Why are we creating a new node attribute rather than following the
existing pattern of using the "custom_query_jumble" attribute on
ArrayExpr and creating a custom jumble function like we do with
_jumbleVariableSetStmt?
Regards,
Sami
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
@ 2025-02-14 08:36 ` Dmitry Dolgov <[email protected]>
2025-02-14 09:28 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Dolgov @ 2025-02-14 08:36 UTC (permalink / raw)
To: Sami Imseih <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
> On Thu, Feb 13, 2025 at 05:08:45PM GMT, Sami Imseih wrote:
> Constants passed as parameters to a prepared statement will not be
> handled as expected. I did not not test explicit PREPARE/EXECUTE statement,
> but I assume it will have the same issue.
This is the same question of supporting various cases. The original
patch implementation handled Param expressions as well, this part was
explicitly rejected during review. I think as a first step it's
important to find a balance between applying this optimization in as
many cases as possible, and at the same time keep the implementation
simple to give the patch a chance. So far I'm inclined to leave Param
for the future work, although of course I'm open to discussion.
> This case with an array passed to aa function seems to cause a regression
> in pg_stat_statements query text. As you can see the text is incomplete.
I've already mentioned that in the previous email. To reiterate, it's
not a functionality regression, but an incomplete representation of a
normalized query which turned out to be hard to change. While I'm
working on that, there is a suggestion that it's not a blocker.
> A typo in the docs.
> c/lenght/length
One day I'll write documentation without any typo, but not today :) Thanks,
will fix with the next version.
> Is this parameter specific to only useful to merge the values of an IN list.
> Should the documentation be more specific and say that only IN lists
> will benefit from this parameter?
You can find one test showing that this optimization is applied to a
plain ARRAY[1, 2, 3, ...], so it's not only IN expressions.
> Also, if there is only 1 value in the list, it will have a different
> queryId than
> that of the same query in which more than 1 value is passed to the IN list.
> Should the documentation be clear about that?
I tend to think there is not much value in emphasizing that. It will add more
mental overhead to process, but this part already says "an array of constants
will contribute only the first and the last elements to the query identifier"
-- having only the first element differs from having both, hence a new entry.
> pg_node_attr of query_jumble_merge is doing something
> very specific to the elements list of an ArrayExpr. The
> merge code likely cannot be used for other node types.
It can be, take a look at pg_node_attr commentary. Any node can have a
field marked with query_jumble_merge attribut and benefit from merging.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-02-14 08:36 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
@ 2025-02-14 09:28 ` Julien Rouhaud <[email protected]>
2025-02-14 09:36 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Julien Rouhaud @ 2025-02-14 09:28 UTC (permalink / raw)
To: Dmitry Dolgov <[email protected]>; +Cc: Sami Imseih <[email protected]>; Álvaro Herrera <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
Hi,
On Fri, Feb 14, 2025 at 09:36:08AM +0100, Dmitry Dolgov wrote:
> > On Thu, Feb 13, 2025 at 05:08:45PM GMT, Sami Imseih wrote:
> > This case with an array passed to aa function seems to cause a regression
> > in pg_stat_statements query text. As you can see the text is incomplete.
>
> I've already mentioned that in the previous email. To reiterate, it's
> not a functionality regression, but an incomplete representation of a
> normalized query which turned out to be hard to change. While I'm
> working on that, there is a suggestion that it's not a blocker.
While talking about the normalized query text with this patch, I see that
merged values are now represented like this, per the regression tests files:
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query | calls
+------------------------------------------------------+-------
+ SELECT * FROM test_merge_numeric WHERE data IN (...) | 1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1
+(2 rows)
This was probably ok a few years back but pg 16 introduced a new GENERIC_PLAN
option for EXPLAIN (3c05284d83b2) to be able to run EXPLAIN on a query
extracted from pg_stat_statements (among other things).
This feature would break the use case. Note that this is not a hypothetical
need: I get very frequent reports on the PoWA project about the impossibility
to get an EXPLAIN (we do have some code that tries to reinject the parameters
from stored quals but we cannot always do it) that is used with the automatic
index suggestion, and we planned to rely on EXPLAIN (GENERIC_PLAN) to have an
always working solution. I suspect that other projects also rely on this
option for similar features.
Since the merging is a yes/no option (I think there used to be some discussions
about having a threshold or some other fancy modes), maybe you could instead
differentiate the merged version by have 2 constants rather than this "..." or
something like that?
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-02-14 08:36 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-14 09:28 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
@ 2025-02-14 09:36 ` Álvaro Herrera <[email protected]>
2025-02-14 09:57 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Álvaro Herrera @ 2025-02-14 09:36 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On 2025-Feb-14, Julien Rouhaud wrote:
> Since the merging is a yes/no option (I think there used to be some discussions
> about having a threshold or some other fancy modes), maybe you could instead
> differentiate the merged version by have 2 constants rather than this "..." or
> something like that?
Maybe the representation can be "($1 /*, ... */)" so that it's obvious
that the array extends beyond the first element but is still
syntactically valid.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
That's because in Europe they call me by name, and in the US by value!"
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: pg_stat_statements and "IN" conditions
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-02-14 08:36 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-14 09:28 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
2025-02-14 09:36 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
@ 2025-02-14 09:57 ` Julien Rouhaud <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Julien Rouhaud @ 2025-02-14 09:57 UTC (permalink / raw)
To: Álvaro Herrera <[email protected]>; +Cc: Dmitry Dolgov <[email protected]>; Sami Imseih <[email protected]>; Kirill Reshke <[email protected]>; Sergei Kornilov <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Sutou Kouhei <[email protected]>
On Fri, Feb 14, 2025 at 10:36:48AM +0100, Álvaro Herrera wrote:
> On 2025-Feb-14, Julien Rouhaud wrote:
>
> > Since the merging is a yes/no option (I think there used to be some discussions
> > about having a threshold or some other fancy modes), maybe you could instead
> > differentiate the merged version by have 2 constants rather than this "..." or
> > something like that?
>
> Maybe the representation can be "($1 /*, ... */)" so that it's obvious
> that the array extends beyond the first element but is still
> syntactically valid.
Yeah that works too and it's probably way easier to implement.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-02-14 09:57 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 12:51 [PATCH v5 1/2] In-place table persistence change Kyotaro Horiguchi <[email protected]>
2025-02-13 16:50 Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-13 23:08 ` Re: pg_stat_statements and "IN" conditions Sami Imseih <[email protected]>
2025-02-14 08:36 ` Re: pg_stat_statements and "IN" conditions Dmitry Dolgov <[email protected]>
2025-02-14 09:28 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
2025-02-14 09:36 ` Re: pg_stat_statements and "IN" conditions Álvaro Herrera <[email protected]>
2025-02-14 09:57 ` Re: pg_stat_statements and "IN" conditions Julien Rouhaud <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox