public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v18 16/18] WIP: Move xid horizon computation for page level index vacuum to primary.
15+ messages / 7 participants
[nested] [flat]
* [PATCH v18 16/18] WIP: Move xid horizon computation for page level index vacuum to primary.
@ 2018-12-19 20:32 Andres Freund <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Andres Freund @ 2018-12-19 20:32 UTC (permalink / raw)
During recovery we do not know which table AM to go to to compute the
xid horizon. To allow for pluggable storage this therefore moves the
computation to the primary.
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/backend/access/hash/hash_xlog.c | 153 +-----------------------
src/backend/access/hash/hashinsert.c | 18 ++-
src/backend/access/heap/heapam.c | 128 ++++++++++++++++++++
src/backend/access/index/genam.c | 36 ++++++
src/backend/access/nbtree/nbtpage.c | 7 ++
src/backend/access/nbtree/nbtxlog.c | 156 +------------------------
src/backend/access/rmgrdesc/hashdesc.c | 5 +-
src/backend/access/rmgrdesc/nbtdesc.c | 3 +-
src/include/access/genam.h | 5 +
src/include/access/hash_xlog.h | 1 +
src/include/access/heapam.h | 4 +
src/include/access/nbtxlog.h | 1 +
12 files changed, 201 insertions(+), 316 deletions(-)
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index c6d87261579..20441e307a8 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -969,155 +969,6 @@ hash_xlog_update_meta_page(XLogReaderState *record)
UnlockReleaseBuffer(metabuf);
}
-/*
- * Get the latestRemovedXid from the heap pages pointed at by the index
- * tuples being deleted. See also btree_xlog_delete_get_latestRemovedXid,
- * on which this function is based.
- */
-static TransactionId
-hash_xlog_vacuum_get_latestRemovedXid(XLogReaderState *record)
-{
- xl_hash_vacuum_one_page *xlrec;
- OffsetNumber *unused;
- Buffer ibuffer,
- hbuffer;
- Page ipage,
- hpage;
- RelFileNode rnode;
- BlockNumber blkno;
- ItemId iitemid,
- hitemid;
- IndexTuple itup;
- HeapTupleHeader htuphdr;
- BlockNumber hblkno;
- OffsetNumber hoffnum;
- TransactionId latestRemovedXid = InvalidTransactionId;
- int i;
-
- xlrec = (xl_hash_vacuum_one_page *) XLogRecGetData(record);
-
- /*
- * If there's nothing running on the standby we don't need to derive a
- * full latestRemovedXid value, so use a fast path out of here. This
- * returns InvalidTransactionId, and so will conflict with all HS
- * transactions; but since we just worked out that that's zero people,
- * it's OK.
- *
- * XXX There is a race condition here, which is that a new backend might
- * start just after we look. If so, it cannot need to conflict, but this
- * coding will result in throwing a conflict anyway.
- */
- if (CountDBBackends(InvalidOid) == 0)
- return latestRemovedXid;
-
- /*
- * Check if WAL replay has reached a consistent database state. If not, we
- * must PANIC. See the definition of
- * btree_xlog_delete_get_latestRemovedXid for more details.
- */
- if (!reachedConsistency)
- elog(PANIC, "hash_xlog_vacuum_get_latestRemovedXid: cannot operate with inconsistent data");
-
- /*
- * Get index page. If the DB is consistent, this should not fail, nor
- * should any of the heap page fetches below. If one does, we return
- * InvalidTransactionId to cancel all HS transactions. That's probably
- * overkill, but it's safe, and certainly better than panicking here.
- */
- XLogRecGetBlockTag(record, 0, &rnode, NULL, &blkno);
- ibuffer = XLogReadBufferExtended(rnode, MAIN_FORKNUM, blkno, RBM_NORMAL);
-
- if (!BufferIsValid(ibuffer))
- return InvalidTransactionId;
- LockBuffer(ibuffer, HASH_READ);
- ipage = (Page) BufferGetPage(ibuffer);
-
- /*
- * Loop through the deleted index items to obtain the TransactionId from
- * the heap items they point to.
- */
- unused = (OffsetNumber *) ((char *) xlrec + SizeOfHashVacuumOnePage);
-
- for (i = 0; i < xlrec->ntuples; i++)
- {
- /*
- * Identify the index tuple about to be deleted.
- */
- iitemid = PageGetItemId(ipage, unused[i]);
- itup = (IndexTuple) PageGetItem(ipage, iitemid);
-
- /*
- * Locate the heap page that the index tuple points at
- */
- hblkno = ItemPointerGetBlockNumber(&(itup->t_tid));
- hbuffer = XLogReadBufferExtended(xlrec->hnode, MAIN_FORKNUM,
- hblkno, RBM_NORMAL);
-
- if (!BufferIsValid(hbuffer))
- {
- UnlockReleaseBuffer(ibuffer);
- return InvalidTransactionId;
- }
- LockBuffer(hbuffer, HASH_READ);
- hpage = (Page) BufferGetPage(hbuffer);
-
- /*
- * Look up the heap tuple header that the index tuple points at by
- * using the heap node supplied with the xlrec. We can't use
- * heap_fetch, since it uses ReadBuffer rather than XLogReadBuffer.
- * Note that we are not looking at tuple data here, just headers.
- */
- hoffnum = ItemPointerGetOffsetNumber(&(itup->t_tid));
- hitemid = PageGetItemId(hpage, hoffnum);
-
- /*
- * Follow any redirections until we find something useful.
- */
- while (ItemIdIsRedirected(hitemid))
- {
- hoffnum = ItemIdGetRedirect(hitemid);
- hitemid = PageGetItemId(hpage, hoffnum);
- CHECK_FOR_INTERRUPTS();
- }
-
- /*
- * If the heap item has storage, then read the header and use that to
- * set latestRemovedXid.
- *
- * Some LP_DEAD items may not be accessible, so we ignore them.
- */
- if (ItemIdHasStorage(hitemid))
- {
- htuphdr = (HeapTupleHeader) PageGetItem(hpage, hitemid);
- HeapTupleHeaderAdvanceLatestRemovedXid(htuphdr, &latestRemovedXid);
- }
- else if (ItemIdIsDead(hitemid))
- {
- /*
- * Conjecture: if hitemid is dead then it had xids before the xids
- * marked on LP_NORMAL items. So we just ignore this item and move
- * onto the next, for the purposes of calculating
- * latestRemovedxids.
- */
- }
- else
- Assert(!ItemIdIsUsed(hitemid));
-
- UnlockReleaseBuffer(hbuffer);
- }
-
- UnlockReleaseBuffer(ibuffer);
-
- /*
- * If all heap tuples were LP_DEAD then we will be returning
- * InvalidTransactionId here, which avoids conflicts. This matches
- * existing logic which assumes that LP_DEAD tuples must already be older
- * than the latestRemovedXid on the cleanup record that set them as
- * LP_DEAD, hence must already have generated a conflict.
- */
- return latestRemovedXid;
-}
-
/*
* replay delete operation in hash index to remove
* tuples marked as DEAD during index tuple insertion.
@@ -1149,12 +1000,10 @@ hash_xlog_vacuum_one_page(XLogReaderState *record)
*/
if (InHotStandby)
{
- TransactionId latestRemovedXid =
- hash_xlog_vacuum_get_latestRemovedXid(record);
RelFileNode rnode;
XLogRecGetBlockTag(record, 0, &rnode, NULL, NULL);
- ResolveRecoveryConflictWithSnapshot(latestRemovedXid, rnode);
+ ResolveRecoveryConflictWithSnapshot(xldata->latestRemovedXid, rnode);
}
action = XLogReadBufferForRedoExtended(record, 0, RBM_NORMAL, true, &buffer);
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 970733f0cd4..a248bd0743f 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -23,8 +23,8 @@
#include "storage/buf_internals.h"
#include "storage/predicate.h"
-static void _hash_vacuum_one_page(Relation rel, Buffer metabuf, Buffer buf,
- RelFileNode hnode);
+static void _hash_vacuum_one_page(Relation rel, Relation hrel,
+ Buffer metabuf, Buffer buf);
/*
* _hash_doinsert() -- Handle insertion of a single index tuple.
@@ -137,7 +137,7 @@ restart_insert:
if (IsBufferCleanupOK(buf))
{
- _hash_vacuum_one_page(rel, metabuf, buf, heapRel->rd_node);
+ _hash_vacuum_one_page(rel, heapRel, metabuf, buf);
if (PageGetFreeSpace(page) >= itemsz)
break; /* OK, now we have enough space */
@@ -335,8 +335,7 @@ _hash_pgaddmultitup(Relation rel, Buffer buf, IndexTuple *itups,
*/
static void
-_hash_vacuum_one_page(Relation rel, Buffer metabuf, Buffer buf,
- RelFileNode hnode)
+_hash_vacuum_one_page(Relation rel, Relation hrel, Buffer metabuf, Buffer buf)
{
OffsetNumber deletable[MaxOffsetNumber];
int ndeletable = 0;
@@ -360,6 +359,12 @@ _hash_vacuum_one_page(Relation rel, Buffer metabuf, Buffer buf,
if (ndeletable > 0)
{
+ TransactionId latestRemovedXid;
+
+ latestRemovedXid =
+ index_compute_xid_horizon_for_tuples(rel, hrel, buf,
+ deletable, ndeletable);
+
/*
* Write-lock the meta page so that we can decrement tuple count.
*/
@@ -393,7 +398,8 @@ _hash_vacuum_one_page(Relation rel, Buffer metabuf, Buffer buf,
xl_hash_vacuum_one_page xlrec;
XLogRecPtr recptr;
- xlrec.hnode = hnode;
+ xlrec.latestRemovedXid = latestRemovedXid;
+ xlrec.hnode = hrel->rd_node;
xlrec.ntuples = ndeletable;
XLogBeginInsert();
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0321d6bab9a..e9fdb0d4cf5 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6946,6 +6946,134 @@ HeapTupleHeaderAdvanceLatestRemovedXid(HeapTupleHeader tuple,
/* *latestRemovedXid may still be invalid at end */
}
+/*
+ * Get the latestRemovedXid from the heap pages pointed at by the index
+ * tuples being deleted.
+ *
+ * This puts the work for calculating latestRemovedXid into the recovery path
+ * rather than the primary path.
+ *
+ * It's possible that this generates a fair amount of I/O, since an index
+ * block may have hundreds of tuples being deleted. To amortize that cost to
+ * some degree, this uses prefetching and combines repeat accesses to the same
+ * block.
+ */
+TransactionId
+heap_compute_xid_horizon_for_tuples(Relation rel,
+ ItemPointerData *tids,
+ int nitems)
+{
+ TransactionId latestRemovedXid = InvalidTransactionId;
+ BlockNumber hblkno;
+ Buffer buf = InvalidBuffer;
+ Page hpage;
+
+ /*
+ * Sort to avoid repeated lookups for the same page, and to make it more
+ * likely to access items in an efficient order. In particular this
+ * ensures thaf if there are multiple pointers to the same page, they all
+ * get processed looking up and locking the page just once.
+ */
+ qsort((void *) tids, nitems, sizeof(ItemPointerData),
+ (int (*) (const void *, const void *)) ItemPointerCompare);
+
+ /* prefetch all pages */
+#ifdef USE_PREFETCH
+ hblkno = InvalidBlockNumber;
+ for (int i = 0; i < nitems; i++)
+ {
+ ItemPointer htid = &tids[i];
+
+ if (hblkno == InvalidBlockNumber ||
+ ItemPointerGetBlockNumber(htid) != hblkno)
+ {
+ hblkno = ItemPointerGetBlockNumber(htid);
+
+ PrefetchBuffer(rel, MAIN_FORKNUM, hblkno);
+ }
+ }
+#endif
+
+ /* Iterate over all tids, and check their horizon */
+ hblkno = InvalidBlockNumber;
+ for (int i = 0; i < nitems; i++)
+ {
+ ItemPointer htid = &tids[i];
+ ItemId hitemid;
+ OffsetNumber hoffnum;
+
+ /*
+ * Read heap buffer, but avoid refetching if it's the same block as
+ * required for the last tid.
+ */
+ if (hblkno == InvalidBlockNumber ||
+ ItemPointerGetBlockNumber(htid) != hblkno)
+ {
+ /* release old buffer */
+ if (BufferIsValid(buf))
+ {
+ LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+ ReleaseBuffer(buf);
+ }
+
+ hblkno = ItemPointerGetBlockNumber(htid);
+
+ buf = ReadBuffer(rel, hblkno);
+ hpage = BufferGetPage(buf);
+
+ LockBuffer(buf, BUFFER_LOCK_SHARE);
+ }
+
+ hoffnum = ItemPointerGetOffsetNumber(htid);
+ hitemid = PageGetItemId(hpage, hoffnum);
+
+ /*
+ * Follow any redirections until we find something useful.
+ */
+ while (ItemIdIsRedirected(hitemid))
+ {
+ hoffnum = ItemIdGetRedirect(hitemid);
+ hitemid = PageGetItemId(hpage, hoffnum);
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ /*
+ * If the heap item has storage, then read the header and use that to
+ * set latestRemovedXid.
+ *
+ * Some LP_DEAD items may not be accessible, so we ignore them.
+ */
+ if (ItemIdHasStorage(hitemid))
+ {
+ HeapTupleHeader htuphdr;
+
+ htuphdr = (HeapTupleHeader) PageGetItem(hpage, hitemid);
+
+ HeapTupleHeaderAdvanceLatestRemovedXid(htuphdr, &latestRemovedXid);
+ }
+ else if (ItemIdIsDead(hitemid))
+ {
+ /*
+ * Conjecture: if hitemid is dead then it had xids before the xids
+ * marked on LP_NORMAL items. So we just ignore this item and move
+ * onto the next, for the purposes of calculating
+ * latestRemovedxids.
+ */
+ }
+ else
+ Assert(!ItemIdIsUsed(hitemid));
+
+ }
+
+ if (BufferIsValid(buf))
+ {
+ LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+ ReleaseBuffer(buf);
+ }
+
+ return latestRemovedXid;
+}
+
/*
* Perform XLogInsert to register a heap cleanup info message. These
* messages are sent once per VACUUM and are required because
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index d34e4ccd3d5..3dbb264e728 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -273,6 +273,42 @@ BuildIndexValueDescription(Relation indexRelation,
return buf.data;
}
+/*
+ * Get the latestRemovedXid from the heap pages pointed at by the index
+ * tuples being deleted.
+ */
+TransactionId
+index_compute_xid_horizon_for_tuples(Relation irel,
+ Relation hrel,
+ Buffer ibuf,
+ OffsetNumber *itemnos,
+ int nitems)
+{
+ ItemPointerData *htids = (ItemPointerData *) palloc(sizeof(ItemPointerData) * nitems);
+ TransactionId latestRemovedXid = InvalidTransactionId;
+ Page ipage = BufferGetPage(ibuf);
+ IndexTuple itup;
+
+ /* identify what the index tuples about to be deleted point to */
+ for (int i = 0; i < nitems; i++)
+ {
+ ItemId iitemid;
+
+ iitemid = PageGetItemId(ipage, itemnos[i]);
+ itup = (IndexTuple) PageGetItem(ipage, iitemid);
+
+ ItemPointerCopy(&itup->t_tid, &htids[i]);
+ }
+
+ /* determine the actual xid horizon */
+ latestRemovedXid =
+ heap_compute_xid_horizon_for_tuples(hrel, htids, nitems);
+
+ pfree(htids);
+
+ return latestRemovedXid;
+}
+
/* ----------------------------------------------------------------
* heap-or-index-scan access to system catalogs
diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c
index 9c785bca95e..bb38bb4606e 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1032,10 +1032,16 @@ _bt_delitems_delete(Relation rel, Buffer buf,
{
Page page = BufferGetPage(buf);
BTPageOpaque opaque;
+ TransactionId latestRemovedXid = InvalidTransactionId;
/* Shouldn't be called unless there's something to do */
Assert(nitems > 0);
+ if (XLogStandbyInfoActive() && RelationNeedsWAL(rel))
+ latestRemovedXid =
+ index_compute_xid_horizon_for_tuples(rel, heapRel, buf,
+ itemnos, nitems);
+
/* No ereport(ERROR) until changes are logged */
START_CRIT_SECTION();
@@ -1065,6 +1071,7 @@ _bt_delitems_delete(Relation rel, Buffer buf,
XLogRecPtr recptr;
xl_btree_delete xlrec_delete;
+ xlrec_delete.latestRemovedXid = latestRemovedXid;
xlrec_delete.hnode = heapRel->rd_node;
xlrec_delete.nitems = nitems;
diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index b0666b42df3..9c277f5016b 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/nbtxlog.c
@@ -518,159 +518,6 @@ btree_xlog_vacuum(XLogReaderState *record)
UnlockReleaseBuffer(buffer);
}
-/*
- * Get the latestRemovedXid from the heap pages pointed at by the index
- * tuples being deleted. This puts the work for calculating latestRemovedXid
- * into the recovery path rather than the primary path.
- *
- * It's possible that this generates a fair amount of I/O, since an index
- * block may have hundreds of tuples being deleted. Repeat accesses to the
- * same heap blocks are common, though are not yet optimised.
- *
- * XXX optimise later with something like XLogPrefetchBuffer()
- */
-static TransactionId
-btree_xlog_delete_get_latestRemovedXid(XLogReaderState *record)
-{
- xl_btree_delete *xlrec = (xl_btree_delete *) XLogRecGetData(record);
- OffsetNumber *unused;
- Buffer ibuffer,
- hbuffer;
- Page ipage,
- hpage;
- RelFileNode rnode;
- BlockNumber blkno;
- ItemId iitemid,
- hitemid;
- IndexTuple itup;
- HeapTupleHeader htuphdr;
- BlockNumber hblkno;
- OffsetNumber hoffnum;
- TransactionId latestRemovedXid = InvalidTransactionId;
- int i;
-
- /*
- * If there's nothing running on the standby we don't need to derive a
- * full latestRemovedXid value, so use a fast path out of here. This
- * returns InvalidTransactionId, and so will conflict with all HS
- * transactions; but since we just worked out that that's zero people,
- * it's OK.
- *
- * XXX There is a race condition here, which is that a new backend might
- * start just after we look. If so, it cannot need to conflict, but this
- * coding will result in throwing a conflict anyway.
- */
- if (CountDBBackends(InvalidOid) == 0)
- return latestRemovedXid;
-
- /*
- * In what follows, we have to examine the previous state of the index
- * page, as well as the heap page(s) it points to. This is only valid if
- * WAL replay has reached a consistent database state; which means that
- * the preceding check is not just an optimization, but is *necessary*. We
- * won't have let in any user sessions before we reach consistency.
- */
- if (!reachedConsistency)
- elog(PANIC, "btree_xlog_delete_get_latestRemovedXid: cannot operate with inconsistent data");
-
- /*
- * Get index page. If the DB is consistent, this should not fail, nor
- * should any of the heap page fetches below. If one does, we return
- * InvalidTransactionId to cancel all HS transactions. That's probably
- * overkill, but it's safe, and certainly better than panicking here.
- */
- XLogRecGetBlockTag(record, 0, &rnode, NULL, &blkno);
- ibuffer = XLogReadBufferExtended(rnode, MAIN_FORKNUM, blkno, RBM_NORMAL);
- if (!BufferIsValid(ibuffer))
- return InvalidTransactionId;
- LockBuffer(ibuffer, BT_READ);
- ipage = (Page) BufferGetPage(ibuffer);
-
- /*
- * Loop through the deleted index items to obtain the TransactionId from
- * the heap items they point to.
- */
- unused = (OffsetNumber *) ((char *) xlrec + SizeOfBtreeDelete);
-
- for (i = 0; i < xlrec->nitems; i++)
- {
- /*
- * Identify the index tuple about to be deleted
- */
- iitemid = PageGetItemId(ipage, unused[i]);
- itup = (IndexTuple) PageGetItem(ipage, iitemid);
-
- /*
- * Locate the heap page that the index tuple points at
- */
- hblkno = ItemPointerGetBlockNumber(&(itup->t_tid));
- hbuffer = XLogReadBufferExtended(xlrec->hnode, MAIN_FORKNUM, hblkno, RBM_NORMAL);
- if (!BufferIsValid(hbuffer))
- {
- UnlockReleaseBuffer(ibuffer);
- return InvalidTransactionId;
- }
- LockBuffer(hbuffer, BT_READ);
- hpage = (Page) BufferGetPage(hbuffer);
-
- /*
- * Look up the heap tuple header that the index tuple points at by
- * using the heap node supplied with the xlrec. We can't use
- * heap_fetch, since it uses ReadBuffer rather than XLogReadBuffer.
- * Note that we are not looking at tuple data here, just headers.
- */
- hoffnum = ItemPointerGetOffsetNumber(&(itup->t_tid));
- hitemid = PageGetItemId(hpage, hoffnum);
-
- /*
- * Follow any redirections until we find something useful.
- */
- while (ItemIdIsRedirected(hitemid))
- {
- hoffnum = ItemIdGetRedirect(hitemid);
- hitemid = PageGetItemId(hpage, hoffnum);
- CHECK_FOR_INTERRUPTS();
- }
-
- /*
- * If the heap item has storage, then read the header and use that to
- * set latestRemovedXid.
- *
- * Some LP_DEAD items may not be accessible, so we ignore them.
- */
- if (ItemIdHasStorage(hitemid))
- {
- htuphdr = (HeapTupleHeader) PageGetItem(hpage, hitemid);
-
- HeapTupleHeaderAdvanceLatestRemovedXid(htuphdr, &latestRemovedXid);
- }
- else if (ItemIdIsDead(hitemid))
- {
- /*
- * Conjecture: if hitemid is dead then it had xids before the xids
- * marked on LP_NORMAL items. So we just ignore this item and move
- * onto the next, for the purposes of calculating
- * latestRemovedxids.
- */
- }
- else
- Assert(!ItemIdIsUsed(hitemid));
-
- UnlockReleaseBuffer(hbuffer);
- }
-
- UnlockReleaseBuffer(ibuffer);
-
- /*
- * If all heap tuples were LP_DEAD then we will be returning
- * InvalidTransactionId here, which avoids conflicts. This matches
- * existing logic which assumes that LP_DEAD tuples must already be older
- * than the latestRemovedXid on the cleanup record that set them as
- * LP_DEAD, hence must already have generated a conflict.
- */
- return latestRemovedXid;
-}
-
static void
btree_xlog_delete(XLogReaderState *record)
{
@@ -693,12 +540,11 @@ btree_xlog_delete(XLogReaderState *record)
*/
if (InHotStandby)
{
- TransactionId latestRemovedXid = btree_xlog_delete_get_latestRemovedXid(record);
RelFileNode rnode;
XLogRecGetBlockTag(record, 0, &rnode, NULL, NULL);
- ResolveRecoveryConflictWithSnapshot(latestRemovedXid, rnode);
+ ResolveRecoveryConflictWithSnapshot(xlrec->latestRemovedXid, rnode);
}
/*
diff --git a/src/backend/access/rmgrdesc/hashdesc.c b/src/backend/access/rmgrdesc/hashdesc.c
index ade1c618161..a29aa96e9ca 100644
--- a/src/backend/access/rmgrdesc/hashdesc.c
+++ b/src/backend/access/rmgrdesc/hashdesc.c
@@ -113,8 +113,9 @@ hash_desc(StringInfo buf, XLogReaderState *record)
{
xl_hash_vacuum_one_page *xlrec = (xl_hash_vacuum_one_page *) rec;
- appendStringInfo(buf, "ntuples %d",
- xlrec->ntuples);
+ appendStringInfo(buf, "ntuples %d, latest removed xid %u",
+ xlrec->ntuples,
+ xlrec->latestRemovedXid);
break;
}
}
diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c
index 8d5c6ae0ab0..64cf7ed02e4 100644
--- a/src/backend/access/rmgrdesc/nbtdesc.c
+++ b/src/backend/access/rmgrdesc/nbtdesc.c
@@ -56,7 +56,8 @@ btree_desc(StringInfo buf, XLogReaderState *record)
{
xl_btree_delete *xlrec = (xl_btree_delete *) rec;
- appendStringInfo(buf, "%d items", xlrec->nitems);
+ appendStringInfo(buf, "%d items, latest removed xid %u",
+ xlrec->nitems, xlrec->latestRemovedXid);
break;
}
case XLOG_BTREE_MARK_PAGE_HALFDEAD:
diff --git a/src/include/access/genam.h b/src/include/access/genam.h
index 1936195c535..780f3255de9 100644
--- a/src/include/access/genam.h
+++ b/src/include/access/genam.h
@@ -187,6 +187,11 @@ extern IndexScanDesc RelationGetIndexScan(Relation indexRelation,
extern void IndexScanEnd(IndexScanDesc scan);
extern char *BuildIndexValueDescription(Relation indexRelation,
Datum *values, bool *isnull);
+extern TransactionId index_compute_xid_horizon_for_tuples(Relation irel,
+ Relation hrel,
+ Buffer ibuf,
+ OffsetNumber *itemnos,
+ int nitems);
/*
* heap-or-index access to system catalogs (in genam.c)
diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h
index 9cef1b7c25d..045e2bf58ba 100644
--- a/src/include/access/hash_xlog.h
+++ b/src/include/access/hash_xlog.h
@@ -263,6 +263,7 @@ typedef struct xl_hash_init_bitmap_page
*/
typedef struct xl_hash_vacuum_one_page
{
+ TransactionId latestRemovedXid;
RelFileNode hnode;
int ntuples;
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index a0cbea9ba00..fefe1daea74 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -181,6 +181,10 @@ extern void simple_heap_update(Relation relation, ItemPointer otid,
extern void heap_sync(Relation relation);
+extern TransactionId heap_compute_xid_horizon_for_tuples(Relation rel,
+ ItemPointerData *items,
+ int nitems);
+
/* in heap/pruneheap.c */
extern void heap_page_prune_opt(Relation relation, Buffer buffer);
extern int heap_page_prune(Relation relation, Buffer buffer,
diff --git a/src/include/access/nbtxlog.h b/src/include/access/nbtxlog.h
index a605851c981..a294bd6fef4 100644
--- a/src/include/access/nbtxlog.h
+++ b/src/include/access/nbtxlog.h
@@ -123,6 +123,7 @@ typedef struct xl_btree_split
*/
typedef struct xl_btree_delete
{
+ TransactionId latestRemovedXid;
RelFileNode hnode; /* RelFileNode of the heap the index currently
* points at */
int nitems;
--
2.21.0.dirty
--yvn3crbc4qf4vymf
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v18-0017-tableam-Add-function-to-determine-newest-xid-amo.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* Add non-blocking version of PQcancel
@ 2022-01-12 15:22 Jelte Fennema <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jelte Fennema @ 2022-01-12 15:22 UTC (permalink / raw)
To: pgsql-hackers
The existing PQcancel API is using blocking IO. This makes PQcancel
impossible to use in an event loop based codebase, without blocking the
event loop until the call returns.
This patch adds a new cancellation API to libpq which is called
PQcancelConnectionStart. This API can be used to send cancellations in a
non-blocking fashion. To do this it internally uses regular PGconn
connection establishment. This has as a downside that
PQcancelConnectionStart cannot be safely called from a signal handler.
Luckily, this should be fine for most usages of this API. Since most
code that's using an event loop handles signals in that event loop as
well (as opposed to calling functions from the signal handler directly).
There are also a few advantages of this approach:
1. No need to add and maintain a second non-blocking connection
establishment codepath.
2. Cancel connections benefit automatically from any improvements made
to the normal connection establishment codepath. Examples of things
that it currently gets for free currently are TLS support and
keepalive settings.
This patch also includes a test for this new API (and also the already
existing cancellation APIs). The test can be easily run like this:
cd src/test/modules/libpq_pipeline
make && ./libpq_pipeline cancel
NOTE: I have not tested this with GSS for the moment. My expectation is
that using this new API with a GSS connection will result in a
CONNECTION_BAD status when calling PQcancelStatus. The reason for this
is that GSS reads will also need to communicate back that an EOF was
found, just like I've done for TLS reads and unencrypted reads. Since in
case of a cancel connection an EOF is actually expected, and should not
be treated as an error.
Attachments:
[application/octet-stream] 0001-Add-non-blocking-version-of-PQcancel.patch (19.0K, ../../AM5PR83MB0178D3B31CA1B6EC4A8ECC42F7529@AM5PR83MB0178.EURPRD83.prod.outlook.com/2-0001-Add-non-blocking-version-of-PQcancel.patch)
download | inline diff:
From 0e0d747c60d564991fc375f439649ac6f35f4578 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Wed, 12 Jan 2022 09:52:05 +0100
Subject: [PATCH] Add non-blocking version of PQcancel
The existing PQcancel API is using blocking IO. This makes PQcancel
impossible to use in an event loop based codebase, without blocking the
event loop until the call returns.
This patch adds a new cancellation API to libpq which is called
PQcancelConnectionStart. This API can be used to send cancellations in a
non-blocking fashion. To do this it internally uses regular PGconn
connection establishment. This has as a downside that
PQcancelConnectionStart cannot be safely called from a signal handler.
Luckily, this should be fine for most usages of this API. Since most
code that's using an event loop handles signals in that event loop as
well (as opposed to calling functions from the signal handler directly).
There are also a few advantages of this approach:
1. No need to add and maintain a second non-blocking connection
establishment codepath.
2. Cancel connections benefit automatically from any improvements made
to the normal connection establishment codepath. Examples of things
that it currently gets for free currently are TLS support and
keepalive settings.
This patch also includes a test for this new API (and also the already
existing cancellation APIs). The test can be easily run like this:
cd src/test/modules/libpq_pipeline
make && ./libpq_pipeline cancel
NOTE: I have not tested this with GSS for the moment. My expectation is
that using this new API with a GSS connection will result in a
CONNECTION_BAD status when calling PQcancelStatus. The reason for this
is that GSS reads will also need to communicate back that an EOF was
found, just like I've done for TLS reads and unencrypted reads. Since in
case of a cancel connection an EOF is actually expected, and should not
be treated as an error.
---
src/interfaces/libpq/exports.txt | 7 +
src/interfaces/libpq/fe-connect.c | 192 +++++++++++++++++-
src/interfaces/libpq/fe-misc.c | 15 +-
src/interfaces/libpq/fe-secure-openssl.c | 2 +-
src/interfaces/libpq/fe-secure.c | 3 +
src/interfaces/libpq/libpq-fe.h | 13 ++
src/interfaces/libpq/libpq-int.h | 8 +
.../modules/libpq_pipeline/libpq_pipeline.c | 115 ++++++++++-
.../libpq_pipeline/t/001_libpq_pipeline.pl | 2 +-
9 files changed, 348 insertions(+), 9 deletions(-)
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index e8bcc88370..64364afeaf 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -186,3 +186,10 @@ PQpipelineStatus 183
PQsetTraceFlags 184
PQmblenBounded 185
PQsendFlushRequest 186
+PQcancelConnect 187
+PQcancelConnectStart 188
+PQcancelConnectPoll 189
+PQcancelStatus 189
+PQcancelSocket 190
+PQcancelErrorMessage 191
+PQcancelFinish 192
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 5fc16be849..347d32ad5f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -378,6 +378,7 @@ static int connectDBComplete(PGconn *conn);
static PGPing internal_ping(PGconn *conn);
static PGconn *makeEmptyPGconn(void);
static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
+static bool copyPGconn(PGconn *srcConn, PGconn *dstConn);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
static void release_conn_addrinfo(PGconn *conn);
@@ -604,8 +605,11 @@ pqDropServerData(PGconn *conn)
if (conn->write_err_msg)
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -737,6 +741,120 @@ PQping(const char *conninfo)
return ret;
}
+/*
+ * PQcancelConnectStart
+ *
+ * Asynchronously cancel a request on the given connection. This requires
+ * polling the returned PGconn to actually complete the cancellation of the
+ * request.
+ */
+PGcancelConn *
+PQcancelConnectStart(PGconn *conn)
+{
+ PGconn *cancelConn = makeEmptyPGconn();
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!copyPGconn(conn, cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Copy over information needed to cancel
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Compute derived options
+ */
+ if (!connectOptions2(cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Connect to the database
+ */
+ if (!connectDBStart(cancelConn))
+ {
+ /* Just in case we failed to set it in connectDBStart */
+ cancelConn->status = CONNECTION_BAD;
+ }
+
+ return (PGcancelConn *) cancelConn;
+}
+
+/*
+ * PQcancelConnect
+ *
+ * Cancel a request on the given connection
+ */
+PGcancelConn *
+PQcancelConnect(PGconn *conn)
+{
+ PGcancelConn *cancelConn = PQcancelConnectStart(conn);
+
+ if (cancelConn && cancelConn->conn.status != CONNECTION_BAD)
+ (void) connectDBComplete(&cancelConn->conn);
+
+ return cancelConn;
+}
+
+/*
+ * PQcancelConnectPoll
+ *
+ * Poll a cancel connection. For usage details see the PQconnectPoll.
+ */
+PostgresPollingStatusType
+PQcancelConnectPoll(PGcancelConn * cancelConn)
+{
+ return PQconnectPoll((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelStatus
+ *
+ * Get the status of a cancel connection.
+ */
+ConnStatusType
+PQcancelStatus(const PGcancelConn * cancelConn)
+{
+ return PQstatus((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelSocket
+ *
+ * Get the socket of the cancel connection.
+ */
+int
+PQcancelSocket(const PGcancelConn * cancelConn)
+{
+ return PQsocket((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelErrorMessage
+ *
+ * Get the socket of the cancel connection.
+ */
+char *
+PQcancelErrorMessage(const PGcancelConn * cancelConn)
+{
+ return PQerrorMessage((PGconn *) cancelConn);
+}
+
+void
+PQcancelFinish(PGcancelConn * cancelConn)
+{
+ PQfinish((PGconn *) cancelConn);
+}
+
+
/*
* PQconnectStartParams
*
@@ -914,6 +1032,46 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+static bool
+copyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ appendPQExpBufferStr(&dstConn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2276,6 +2434,17 @@ PQconnectPoll(PGconn *conn)
/* Load waiting data */
int n = pqReadData(conn);
+ if (n == -2 && conn->cancelRequest)
+ {
+ /*
+ * This is the expected end state for cancel connections.
+ * They are closed once the cancel is processed by the
+ * server.
+ */
+ conn->status = CONNECTION_CANCEL_FINISHED;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+ }
if (n < 0)
goto error_return;
if (n == 0)
@@ -2950,6 +3119,25 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not send cancel packet: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 7fcfe08fd2..a95d63ffcd 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -558,8 +558,11 @@ pqPutMsgEnd(PGconn *conn)
* Possible return values:
* 1: successfully loaded at least one more byte
* 0: no data is presently available, but no error detected
- * -1: error detected (including EOF = connection closure);
+ * -1: error detected (excluding EOF = connection closure);
* conn->errorMessage set
+ * -2: EOF detected, connection is closed
+ * conn->errorMessage set
+ *
* NOTE: callers must not assume that pointers or indexes into conn->inBuffer
* remain valid across this call!
* ----------
@@ -642,7 +645,7 @@ retry3:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -737,7 +740,7 @@ retry4:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -755,13 +758,17 @@ definitelyEOF:
libpq_gettext("server closed the connection unexpectedly\n"
"\tThis probably means the server terminated abnormally\n"
"\tbefore or while processing the request.\n"));
+ /* Do *not* drop any already-read data; caller still wants it */
+ pqDropConnection(conn, false);
+ conn->status = CONNECTION_BAD; /* No more connection to backend */
+ return -2;
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
/* Do *not* drop any already-read data; caller still wants it */
pqDropConnection(conn, false);
conn->status = CONNECTION_BAD; /* No more connection to backend */
- return -1;
+ return nread < 0 ? nread : -1;
}
/*
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 9f735ba437..3cd65fa276 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -252,7 +252,7 @@ rloop:
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("SSL connection has been closed unexpectedly\n"));
result_errno = ECONNRESET;
- n = -1;
+ n = -2;
break;
default:
appendPQExpBuffer(&conn->errorMessage,
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 0b998e254d..b2c66f47a5 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -201,6 +201,9 @@ pqsecure_close(PGconn *conn)
* On failure, this function is responsible for appending a suitable message
* to conn->errorMessage. The caller must still inspect errno, but only
* to determine whether to continue/retry after error.
+ *
+ * Returns -1 in case of failures, except in the case of clean connection
+ * closure then it returns -2.
*/
ssize_t
pqsecure_read(PGconn *conn, void *ptr, size_t len)
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 20eb855abc..39aed5db3e 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -57,6 +57,7 @@ typedef enum
{
CONNECTION_OK,
CONNECTION_BAD,
+ CONNECTION_CANCEL_FINISHED,
/* Non-blocking mode only below here */
/*
@@ -163,6 +164,11 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+typedef struct pg_cancel_conn PGcancelConn;
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -327,6 +333,13 @@ extern void PQfreeCancel(PGcancel *cancel);
/* issue a cancel request */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
+extern PGcancelConn * PQcancelConnectStart(PGconn *conn);
+extern PGcancelConn * PQcancelConnect(PGconn *conn);
+extern PostgresPollingStatusType PQcancelConnectPoll(PGcancelConn * cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
+extern int PQcancelSocket(const PGcancelConn * cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
+extern void PQcancelFinish(PGcancelConn * cancelConn);
/* backwards compatible version of PQcancel; not thread-safe */
extern int PQrequestCancel(PGconn *conn);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index fcce13843e..8af3dd0ee7 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -394,6 +394,8 @@ struct pg_conn
char *ssl_max_protocol_version; /* maximum TLS protocol version */
char *target_session_attrs; /* desired session properties */
+ bool cancelRequest;
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
@@ -574,6 +576,11 @@ struct pg_conn
PQExpBufferData workBuffer; /* expansible string */
};
+struct pg_cancel_conn
+{
+ PGconn conn;
+};
+
/* PGcancel stores all data necessary to cancel a connection. A copy of this
* data is required to safely cancel a connection running on a different
* thread.
@@ -691,6 +698,7 @@ extern int pqPutInt(int value, size_t bytes, PGconn *conn);
extern int pqPutMsgStart(char msg_type, PGconn *conn);
extern int pqPutMsgEnd(PGconn *conn);
extern int pqReadData(PGconn *conn);
+extern int pqReadDataOrEof(PGconn *conn);
extern int pqFlush(PGconn *conn);
extern int pqWait(int forRead, int forWrite, PGconn *conn);
extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn,
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 0ff563f59a..27188d43bb 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -86,6 +86,116 @@ pg_fatal_impl(int line, const char *fmt,...)
exit(1);
}
+static void
+confirm_query_cancelled(PGconn *conn)
+{
+ PGresult *res = NULL;
+
+ res = PQgetResult(conn);
+ if (res == NULL)
+ pg_fatal("PQgetResult returned null: %s",
+ PQerrorMessage(conn));
+ if (PQresultStatus(res) != PGRES_FATAL_ERROR)
+ pg_fatal("query did not fail when it was expected");
+ if (strcmp(PQresultErrorField(res, PG_DIAG_SQLSTATE), "57014") != 0)
+ pg_fatal("query failed with a different error than cancellation: %s", PQerrorMessage(conn));
+ PQclear(res);
+ while (PQisBusy(conn))
+ {
+ PQconsumeInput(conn);
+ }
+}
+
+static void
+test_cancel(PGconn *conn)
+{
+ PGcancel *cancel = NULL;
+ PGcancelConn *cancelConn = NULL;
+ char errorbuf[256];
+
+ if (PQsetnonblocking(conn, 1) != 0)
+ pg_fatal("failed to set nonblocking mode: %s", PQerrorMessage(conn));
+
+ /* test PQrequestcancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ PQrequestCancel(conn);
+ confirm_query_cancelled(conn);
+
+ /* test PQcancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancel = PQgetCancel(conn);
+ PQcancel(cancel, errorbuf, sizeof(errorbuf));
+ confirm_query_cancelled(conn);
+
+ /* test PQcancelConnect */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQcancelConnect(conn);
+ if (PQcancelStatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+ PQcancelFinish(cancelConn);
+
+ /* test PQcancelConnectStart and then polling with PQcancelConnectPoll */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQcancelConnectStart(conn);
+ if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelConnectPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+ PQcancelFinish(cancelConn);
+}
+
static void
test_disallowed_in_pipeline(PGconn *conn)
{
@@ -1555,6 +1665,7 @@ print_test_list(void)
printf("singlerow\n");
printf("transaction\n");
printf("uniqviol\n");
+ printf("cancel\n");
}
int
@@ -1642,7 +1753,9 @@ main(int argc, char **argv)
PQTRACE_SUPPRESS_TIMESTAMPS | PQTRACE_REGRESS_MODE);
}
- if (strcmp(testname, "disallowed_in_pipeline") == 0)
+ if (strcmp(testname, "cancel") == 0)
+ test_cancel(conn);
+ else if (strcmp(testname, "disallowed_in_pipeline") == 0)
test_disallowed_in_pipeline(conn);
else if (strcmp(testname, "multi_pipelines") == 0)
test_multi_pipelines(conn);
diff --git a/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl b/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl
index 0c164dcaba..e0773543ae 100644
--- a/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl
+++ b/src/test/modules/libpq_pipeline/t/001_libpq_pipeline.pl
@@ -26,7 +26,7 @@ for my $testname (@tests)
my @extraargs = ('-r', $numrows);
my $cmptrace = grep(/^$testname$/,
qw(simple_pipeline nosync multi_pipelines prepared singlerow
- pipeline_abort transaction disallowed_in_pipeline)) > 0;
+ pipeline_abort transaction disallowed_in_pipeline cancel)) > 0;
# For a bunch of tests, generate a libpq trace file too.
my $traceout = "$PostgreSQL::Test::Utils::tmp_check/traces/$testname.trace";
--
2.17.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-01-13 00:44 Andres Freund <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-01-13 00:44 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: pgsql-hackers
Hi,
On 2022-01-12 15:22:18 +0000, Jelte Fennema wrote:
> This patch also includes a test for this new API (and also the already
> existing cancellation APIs). The test can be easily run like this:
>
> cd src/test/modules/libpq_pipeline
> make && ./libpq_pipeline cancel
Right now tests fails to build on windows with:
[15:45:10.518] src/interfaces/libpq/libpqdll.def : fatal error LNK1121: duplicate ordinal number '189' [c:\cirrus\libpq.vcxproj]
on fails tests on other platforms. See
https://cirrus-ci.com/build/4791821363576832
> NOTE: I have not tested this with GSS for the moment. My expectation is
> that using this new API with a GSS connection will result in a
> CONNECTION_BAD status when calling PQcancelStatus. The reason for this
> is that GSS reads will also need to communicate back that an EOF was
> found, just like I've done for TLS reads and unencrypted reads. Since in
> case of a cancel connection an EOF is actually expected, and should not
> be treated as an error.
The failures do not seem related to this.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-01-13 14:51 Jelte Fennema <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jelte Fennema @ 2022-01-13 14:51 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
Attached is an updated patch which I believe fixes windows and the other test failures.
At least on my machine make check-world passes now when compiled with --enable-tap-tests
I also included a second patch which adds some basic documentation for the libpq tests.
Attachments:
[application/octet-stream] 0001-Add-non-blocking-version-of-PQcancel.patch (18.2K, ../../AM5PR83MB0178CE0CEB690F1C041355B7F7539@AM5PR83MB0178.EURPRD83.prod.outlook.com/3-0001-Add-non-blocking-version-of-PQcancel.patch)
download | inline diff:
From 3619ddcfc11d7d0cc39cb6dc12a8561fb895b385 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Wed, 12 Jan 2022 09:52:05 +0100
Subject: [PATCH 1/2] Add non-blocking version of PQcancel
The existing PQcancel API is using blocking IO. This makes PQcancel
impossible to use in an event loop based codebase, without blocking the
event loop until the call returns.
This patch adds a new cancellation API to libpq which is called
PQcancelConnectionStart. This API can be used to send cancellations in a
non-blocking fashion. To do this it internally uses regular PGconn
connection establishment. This has as a downside that
PQcancelConnectionStart cannot be safely called from a signal handler.
Luckily, this should be fine for most usages of this API. Since most
code that's using an event loop handles signals in that event loop as
well (as opposed to calling functions from the signal handler directly).
There are also a few advantages of this approach:
1. No need to add and maintain a second non-blocking connection
establishment codepath.
2. Cancel connections benefit automatically from any improvements made
to the normal connection establishment codepath. Examples of things
that it currently gets for free currently are TLS support and
keepalive settings.
This patch also includes a test for this new API (and also the already
existing cancellation APIs). The test can be easily run like this:
cd src/test/modules/libpq_pipeline
make && ./libpq_pipeline cancel
NOTE: I have not tested this with GSS for the moment. My expectation is
that using this new API with a GSS connection will result in a
CONNECTION_BAD status when calling PQcancelStatus. The reason for this
is that GSS reads will also need to communicate back that an EOF was
found, just like I've done for TLS reads and unencrypted reads. Since in
case of a cancel connection an EOF is actually expected, and should not
be treated as an error.
---
src/interfaces/libpq/exports.txt | 7 +
src/interfaces/libpq/fe-connect.c | 192 +++++++++++++++++-
src/interfaces/libpq/fe-misc.c | 15 +-
src/interfaces/libpq/fe-secure-openssl.c | 2 +-
src/interfaces/libpq/fe-secure.c | 3 +
src/interfaces/libpq/libpq-fe.h | 13 ++
src/interfaces/libpq/libpq-int.h | 8 +
.../modules/libpq_pipeline/libpq_pipeline.c | 115 ++++++++++-
8 files changed, 347 insertions(+), 8 deletions(-)
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index e8bcc88370..a06214a78f 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -186,3 +186,10 @@ PQpipelineStatus 183
PQsetTraceFlags 184
PQmblenBounded 185
PQsendFlushRequest 186
+PQcancelConnect 187
+PQcancelConnectStart 188
+PQcancelConnectPoll 189
+PQcancelStatus 190
+PQcancelSocket 191
+PQcancelErrorMessage 192
+PQcancelFinish 193
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 5fc16be849..347d32ad5f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -378,6 +378,7 @@ static int connectDBComplete(PGconn *conn);
static PGPing internal_ping(PGconn *conn);
static PGconn *makeEmptyPGconn(void);
static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
+static bool copyPGconn(PGconn *srcConn, PGconn *dstConn);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
static void release_conn_addrinfo(PGconn *conn);
@@ -604,8 +605,11 @@ pqDropServerData(PGconn *conn)
if (conn->write_err_msg)
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -737,6 +741,120 @@ PQping(const char *conninfo)
return ret;
}
+/*
+ * PQcancelConnectStart
+ *
+ * Asynchronously cancel a request on the given connection. This requires
+ * polling the returned PGconn to actually complete the cancellation of the
+ * request.
+ */
+PGcancelConn *
+PQcancelConnectStart(PGconn *conn)
+{
+ PGconn *cancelConn = makeEmptyPGconn();
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!copyPGconn(conn, cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Copy over information needed to cancel
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Compute derived options
+ */
+ if (!connectOptions2(cancelConn))
+ return (PGcancelConn *) cancelConn;
+
+ /*
+ * Connect to the database
+ */
+ if (!connectDBStart(cancelConn))
+ {
+ /* Just in case we failed to set it in connectDBStart */
+ cancelConn->status = CONNECTION_BAD;
+ }
+
+ return (PGcancelConn *) cancelConn;
+}
+
+/*
+ * PQcancelConnect
+ *
+ * Cancel a request on the given connection
+ */
+PGcancelConn *
+PQcancelConnect(PGconn *conn)
+{
+ PGcancelConn *cancelConn = PQcancelConnectStart(conn);
+
+ if (cancelConn && cancelConn->conn.status != CONNECTION_BAD)
+ (void) connectDBComplete(&cancelConn->conn);
+
+ return cancelConn;
+}
+
+/*
+ * PQcancelConnectPoll
+ *
+ * Poll a cancel connection. For usage details see the PQconnectPoll.
+ */
+PostgresPollingStatusType
+PQcancelConnectPoll(PGcancelConn * cancelConn)
+{
+ return PQconnectPoll((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelStatus
+ *
+ * Get the status of a cancel connection.
+ */
+ConnStatusType
+PQcancelStatus(const PGcancelConn * cancelConn)
+{
+ return PQstatus((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelSocket
+ *
+ * Get the socket of the cancel connection.
+ */
+int
+PQcancelSocket(const PGcancelConn * cancelConn)
+{
+ return PQsocket((PGconn *) cancelConn);
+}
+
+/*
+ * PQcancelErrorMessage
+ *
+ * Get the socket of the cancel connection.
+ */
+char *
+PQcancelErrorMessage(const PGcancelConn * cancelConn)
+{
+ return PQerrorMessage((PGconn *) cancelConn);
+}
+
+void
+PQcancelFinish(PGcancelConn * cancelConn)
+{
+ PQfinish((PGconn *) cancelConn);
+}
+
+
/*
* PQconnectStartParams
*
@@ -914,6 +1032,46 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+static bool
+copyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ appendPQExpBufferStr(&dstConn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2276,6 +2434,17 @@ PQconnectPoll(PGconn *conn)
/* Load waiting data */
int n = pqReadData(conn);
+ if (n == -2 && conn->cancelRequest)
+ {
+ /*
+ * This is the expected end state for cancel connections.
+ * They are closed once the cancel is processed by the
+ * server.
+ */
+ conn->status = CONNECTION_CANCEL_FINISHED;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+ }
if (n < 0)
goto error_return;
if (n == 0)
@@ -2950,6 +3119,25 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not send cancel packet: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 7fcfe08fd2..a95d63ffcd 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -558,8 +558,11 @@ pqPutMsgEnd(PGconn *conn)
* Possible return values:
* 1: successfully loaded at least one more byte
* 0: no data is presently available, but no error detected
- * -1: error detected (including EOF = connection closure);
+ * -1: error detected (excluding EOF = connection closure);
* conn->errorMessage set
+ * -2: EOF detected, connection is closed
+ * conn->errorMessage set
+ *
* NOTE: callers must not assume that pointers or indexes into conn->inBuffer
* remain valid across this call!
* ----------
@@ -642,7 +645,7 @@ retry3:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -737,7 +740,7 @@ retry4:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -755,13 +758,17 @@ definitelyEOF:
libpq_gettext("server closed the connection unexpectedly\n"
"\tThis probably means the server terminated abnormally\n"
"\tbefore or while processing the request.\n"));
+ /* Do *not* drop any already-read data; caller still wants it */
+ pqDropConnection(conn, false);
+ conn->status = CONNECTION_BAD; /* No more connection to backend */
+ return -2;
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
/* Do *not* drop any already-read data; caller still wants it */
pqDropConnection(conn, false);
conn->status = CONNECTION_BAD; /* No more connection to backend */
- return -1;
+ return nread < 0 ? nread : -1;
}
/*
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 9f735ba437..3cd65fa276 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -252,7 +252,7 @@ rloop:
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("SSL connection has been closed unexpectedly\n"));
result_errno = ECONNRESET;
- n = -1;
+ n = -2;
break;
default:
appendPQExpBuffer(&conn->errorMessage,
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 0b998e254d..b2c66f47a5 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -201,6 +201,9 @@ pqsecure_close(PGconn *conn)
* On failure, this function is responsible for appending a suitable message
* to conn->errorMessage. The caller must still inspect errno, but only
* to determine whether to continue/retry after error.
+ *
+ * Returns -1 in case of failures, except in the case of clean connection
+ * closure then it returns -2.
*/
ssize_t
pqsecure_read(PGconn *conn, void *ptr, size_t len)
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 20eb855abc..39aed5db3e 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -57,6 +57,7 @@ typedef enum
{
CONNECTION_OK,
CONNECTION_BAD,
+ CONNECTION_CANCEL_FINISHED,
/* Non-blocking mode only below here */
/*
@@ -163,6 +164,11 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+typedef struct pg_cancel_conn PGcancelConn;
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -327,6 +333,13 @@ extern void PQfreeCancel(PGcancel *cancel);
/* issue a cancel request */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
+extern PGcancelConn * PQcancelConnectStart(PGconn *conn);
+extern PGcancelConn * PQcancelConnect(PGconn *conn);
+extern PostgresPollingStatusType PQcancelConnectPoll(PGcancelConn * cancelConn);
+extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
+extern int PQcancelSocket(const PGcancelConn * cancelConn);
+extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
+extern void PQcancelFinish(PGcancelConn * cancelConn);
/* backwards compatible version of PQcancel; not thread-safe */
extern int PQrequestCancel(PGconn *conn);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index fcce13843e..8af3dd0ee7 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -394,6 +394,8 @@ struct pg_conn
char *ssl_max_protocol_version; /* maximum TLS protocol version */
char *target_session_attrs; /* desired session properties */
+ bool cancelRequest;
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
@@ -574,6 +576,11 @@ struct pg_conn
PQExpBufferData workBuffer; /* expansible string */
};
+struct pg_cancel_conn
+{
+ PGconn conn;
+};
+
/* PGcancel stores all data necessary to cancel a connection. A copy of this
* data is required to safely cancel a connection running on a different
* thread.
@@ -691,6 +698,7 @@ extern int pqPutInt(int value, size_t bytes, PGconn *conn);
extern int pqPutMsgStart(char msg_type, PGconn *conn);
extern int pqPutMsgEnd(PGconn *conn);
extern int pqReadData(PGconn *conn);
+extern int pqReadDataOrEof(PGconn *conn);
extern int pqFlush(PGconn *conn);
extern int pqWait(int forRead, int forWrite, PGconn *conn);
extern int pqWaitTimed(int forRead, int forWrite, PGconn *conn,
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 0ff563f59a..27188d43bb 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -86,6 +86,116 @@ pg_fatal_impl(int line, const char *fmt,...)
exit(1);
}
+static void
+confirm_query_cancelled(PGconn *conn)
+{
+ PGresult *res = NULL;
+
+ res = PQgetResult(conn);
+ if (res == NULL)
+ pg_fatal("PQgetResult returned null: %s",
+ PQerrorMessage(conn));
+ if (PQresultStatus(res) != PGRES_FATAL_ERROR)
+ pg_fatal("query did not fail when it was expected");
+ if (strcmp(PQresultErrorField(res, PG_DIAG_SQLSTATE), "57014") != 0)
+ pg_fatal("query failed with a different error than cancellation: %s", PQerrorMessage(conn));
+ PQclear(res);
+ while (PQisBusy(conn))
+ {
+ PQconsumeInput(conn);
+ }
+}
+
+static void
+test_cancel(PGconn *conn)
+{
+ PGcancel *cancel = NULL;
+ PGcancelConn *cancelConn = NULL;
+ char errorbuf[256];
+
+ if (PQsetnonblocking(conn, 1) != 0)
+ pg_fatal("failed to set nonblocking mode: %s", PQerrorMessage(conn));
+
+ /* test PQrequestcancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ PQrequestCancel(conn);
+ confirm_query_cancelled(conn);
+
+ /* test PQcancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancel = PQgetCancel(conn);
+ PQcancel(cancel, errorbuf, sizeof(errorbuf));
+ confirm_query_cancelled(conn);
+
+ /* test PQcancelConnect */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQcancelConnect(conn);
+ if (PQcancelStatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+ PQcancelFinish(cancelConn);
+
+ /* test PQcancelConnectStart and then polling with PQcancelConnectPoll */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQcancelConnectStart(conn);
+ if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQcancelConnectPoll(cancelConn);
+ int sock = PQcancelSocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQcancelErrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQcancelStatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQcancelErrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+ PQcancelFinish(cancelConn);
+}
+
static void
test_disallowed_in_pipeline(PGconn *conn)
{
@@ -1555,6 +1665,7 @@ print_test_list(void)
printf("singlerow\n");
printf("transaction\n");
printf("uniqviol\n");
+ printf("cancel\n");
}
int
@@ -1642,7 +1753,9 @@ main(int argc, char **argv)
PQTRACE_SUPPRESS_TIMESTAMPS | PQTRACE_REGRESS_MODE);
}
- if (strcmp(testname, "disallowed_in_pipeline") == 0)
+ if (strcmp(testname, "cancel") == 0)
+ test_cancel(conn);
+ else if (strcmp(testname, "disallowed_in_pipeline") == 0)
test_disallowed_in_pipeline(conn);
else if (strcmp(testname, "multi_pipelines") == 0)
test_multi_pipelines(conn);
--
2.17.1
[application/octet-stream] 0002-Add-documentation-for-libpq_pipeline-tests.patch (1.5K, ../../AM5PR83MB0178CE0CEB690F1C041355B7F7539@AM5PR83MB0178.EURPRD83.prod.outlook.com/4-0002-Add-documentation-for-libpq_pipeline-tests.patch)
download | inline diff:
From e59f8174d5d006e78e474873ca4ba89a4e5c21d2 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Thu, 13 Jan 2022 15:26:35 +0100
Subject: [PATCH 2/2] Add documentation for libpq_pipeline tests
This adds some explanation on how to run and add libpq tests.
---
src/test/modules/libpq_pipeline/README | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/test/modules/libpq_pipeline/README b/src/test/modules/libpq_pipeline/README
index d8174dd579..3eac0bf131 100644
--- a/src/test/modules/libpq_pipeline/README
+++ b/src/test/modules/libpq_pipeline/README
@@ -1 +1,21 @@
Test programs and libraries for libpq
+=====================================
+
+You can manually run a specific test by running:
+
+ ./libpq_pipeline <name of test>
+
+You can add new tests to libpq_pipeline.c to adding a new test for libpq you
+should add your new test you should add the name of your new test to the
+"print_test_list" function. Then in main you should do something when this test
+name is passed to the program.
+
+If the order in which postgres protocol messages are sent is deterministic for
+your test. Then you can generate a trace of these messages using the following
+command:
+
+ ./libpq_pipeline mynewtest -t traces/mynewtest.trace
+
+Once you've done that you should make sure that when running "make check"
+the generated trace is compared to the expected trace. This is done by adding
+your test name to the $cmptrace definition in the t/001_libpq_pipeline.pl file
--
2.17.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-09 00:27 Jacob Champion <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jacob Champion @ 2022-03-09 00:27 UTC (permalink / raw)
To: [email protected] <[email protected]>; [email protected] <[email protected]>; +Cc: pgsql-hackers
On Thu, 2022-01-13 at 14:51 +0000, Jelte Fennema wrote:
> Attached is an updated patch which I believe fixes windows and the other test failures.
> At least on my machine make check-world passes now when compiled with --enable-tap-tests
>
> I also included a second patch which adds some basic documentation for the libpq tests.
This is not a full review by any means, but here are my thoughts so
far:
> NOTE: I have not tested this with GSS for the moment. My expectation is
> that using this new API with a GSS connection will result in a
> CONNECTION_BAD status when calling PQcancelStatus. The reason for this
> is that GSS reads will also need to communicate back that an EOF was
> found, just like I've done for TLS reads and unencrypted reads.
For what it's worth, I did a smoke test with a Kerberos environment via
./libpq_pipeline cancel '... gssencmode=require'
and the tests claim to pass.
> 2. Cancel connections benefit automatically from any improvements made
> to the normal connection establishment codepath. Examples of things
> that it currently gets for free currently are TLS support and
> keepalive settings.
This seems like a big change compared to PQcancel(); one that's not
really hinted at elsewhere. Having the async version of an API open up
a completely different code path with new features is pretty surprising
to me.
And does the backend actually handle cancel requests via TLS (or GSS)?
It didn't look that way from a quick scan, but I may have missed
something.
> @@ -1555,6 +1665,7 @@ print_test_list(void)
> printf("singlerow\n");
> printf("transaction\n");
> printf("uniqviol\n");
> + printf("cancel\n");
> }
This should probably go near the top; it looks like the existing list
is alphabetized.
The new cancel tests don't print any feedback. It'd be nice to get the
same sort of output as the other tests.
> /* issue a cancel request */
> extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
> +extern PGcancelConn * PQcancelConnectStart(PGconn *conn);
> +extern PGcancelConn * PQcancelConnect(PGconn *conn);
> +extern PostgresPollingStatusType PQcancelConnectPoll(PGcancelConn * cancelConn);
> +extern ConnStatusType PQcancelStatus(const PGcancelConn * cancelConn);
> +extern int PQcancelSocket(const PGcancelConn * cancelConn);
> +extern char *PQcancelErrorMessage(const PGcancelConn * cancelConn);
> +extern void PQcancelFinish(PGcancelConn * cancelConn);
That's a lot of new entry points, most of which don't do anything
except call their twin after a pointer cast. How painful would it be to
just use the existing APIs as-is, and error out when calling
unsupported functions if conn->cancelRequest is true?
--Jacob
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-24 21:41 Tom Lane <[email protected]>
parent: Jacob Champion <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-03-24 21:41 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected]
Jacob Champion <[email protected]> writes:
> On Thu, 2022-01-13 at 14:51 +0000, Jelte Fennema wrote:
>> 2. Cancel connections benefit automatically from any improvements made
>> to the normal connection establishment codepath. Examples of things
>> that it currently gets for free currently are TLS support and
>> keepalive settings.
> This seems like a big change compared to PQcancel(); one that's not
> really hinted at elsewhere. Having the async version of an API open up
> a completely different code path with new features is pretty surprising
> to me.
Well, the patch lacks any user-facing doco at all, so a-fortiori this
point is not covered. I trust the plan was to write docs later.
I kind of feel that this patch is going in the wrong direction.
I do see the need for a version of PQcancel that can encrypt the
transmitted cancel request (and yes, that should work on the backend
side; see recursion in ProcessStartupPacket). I have not seen
requests for a non-blocking version, and this doesn't surprise me.
I feel that the whole non-blocking aspect of libpq probably belongs
to another era when people didn't trust threads.
So what I'd do is make a version that just takes a PGconn, sends the
cancel request, and returns success or failure; never mind the
non-blocking aspect. One possible long-run advantage of this is that
it might be possible to "sync" the cancel request so that we know,
or at least can find out afterwards, exactly which query got
cancelled; something that's fundamentally impossible if the cancel
function works from a clone data structure that is disconnected
from the current connection state.
(Note that it probably makes sense to make a clone PGconn to pass
to fe-connect.c, internally to this function. I just don't want
to expose that to the app.)
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-24 22:49 Andres Freund <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-03-24 22:49 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Jacob Champion <[email protected]>; [email protected] <[email protected]>; [email protected]
Hi,
On 2022-03-24 17:41:53 -0400, Tom Lane wrote:
> I kind of feel that this patch is going in the wrong direction.
> I do see the need for a version of PQcancel that can encrypt the
> transmitted cancel request (and yes, that should work on the backend
> side; see recursion in ProcessStartupPacket). I have not seen
> requests for a non-blocking version, and this doesn't surprise me.
> I feel that the whole non-blocking aspect of libpq probably belongs
> to another era when people didn't trust threads.
That's not a whole lot of fun if you think of cases like postgres_fdw (or
citus as in Jelte's case), which run inside the backend. Even with just a
single postgres_fdw, we don't really want to end up in an uninterruptible
PQcancel() that doesn't even react to pg_terminate_backend().
Even if using threads weren't an issue, I don't really buy the premise - most
networking code has moved *away* from using dedicated threads for each
connection. It just doesn't scale.
Leaving PQcancel aside, we use the non-blocking libpq stuff widely
ourselves. I think walreceiver, isolationtester, pgbench etc would be *much*
harder to get working equally well if there was just blocking calls. If
anything, we're getting to the point where purely blocking functionality
shouldn't be added anymore.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-25 18:34 Robert Haas <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Haas @ 2022-03-25 18:34 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Jacob Champion <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Mar 24, 2022 at 6:49 PM Andres Freund <[email protected]> wrote:
> That's not a whole lot of fun if you think of cases like postgres_fdw (or
> citus as in Jelte's case), which run inside the backend. Even with just a
> single postgres_fdw, we don't really want to end up in an uninterruptible
> PQcancel() that doesn't even react to pg_terminate_backend().
>
> Even if using threads weren't an issue, I don't really buy the premise - most
> networking code has moved *away* from using dedicated threads for each
> connection. It just doesn't scale.
>
> Leaving PQcancel aside, we use the non-blocking libpq stuff widely
> ourselves. I think walreceiver, isolationtester, pgbench etc would be *much*
> harder to get working equally well if there was just blocking calls. If
> anything, we're getting to the point where purely blocking functionality
> shouldn't be added anymore.
+1. I think having a non-blocking version of PQcancel() available is a
great idea, and I've wanted it myself. See commit
ae9bfc5d65123aaa0d1cca9988037489760bdeae.
That said, I don't think that this particular patch is going in the
right direction. I think Jacob's comment upthread is right on point:
"This seems like a big change compared to PQcancel(); one that's not
really hinted at elsewhere. Having the async version of an API open up
a completely different code path with new features is pretty
surprising to me." It seems to me that we want to end up with similar
code paths for PQcancel() and the non-blocking version of cancel. We
could get there in two ways. One way would be to implement the
non-blocking functionality in a manner that matches exactly what
PQcancel() does now. I imagine that the existing code from PQcancel()
would move, with some amount of change, into a new set of non-blocking
APIs. Perhaps PQcancel() would then be rewritten to use those new APIs
instead of hand-rolling the same logic. The other possible approach
would be to first change the blocking version of PQcancel() to use the
regular connection code instead of its own idiosyncratic logic, and
then as a second step, extend it with non-blocking interfaces that use
the regular non-blocking connection code. With either of these
approaches, we end up with the functionality working similarly in the
blocking and non-blocking code paths.
Leaving the question of approach aside, I think it's fairly clear that
this patch cannot be seriously considered for v15. One problem is the
lack of user-facing documentation, but there's a other stuff that just
doesn't look sufficiently well-considered. For example, it updates the
comment for pqsecure_read() to say "Returns -1 in case of failures,
except in the case of clean connection closure then it returns -2."
But that function calls any of three different implementation
functions depending on the situation and the patch only updates one of
them. And it updates that function to return -2 when the is
ECONNRESET, which seems to fly in the face of the comment's idea that
this is the "clean connection closure" case. I think it's probably a
bad sign that this function is tinkering with logic in this sort of
low-level function anyway. pqReadData() is a really general function
that manages to work with non-blocking I/O already, so why does
non-blocking query cancellation need to change its return values, or
whether or not it drops data in certain cases?
I'm also skeptical about the fact that we end up with a whole bunch of
new functions that are just wrappers around existing functions. That's
not a scalable approach. Every function that we have for a PGconn will
eventually need a variant that deals with a PGcancelConn. That seems
kind of pointless, especially considering that a PGcancelConn is
*exactly* a PGconn in disguise. If we decide to pursue the approach of
using the existing infrastructure for PGconn objects to handle query
cancellation, we ought to manipulate them using the same functions we
currently do, with some kind of mode or flag or switch or something
that you can use to turn a regular PGconn into something that cancels
a query. Maybe you create the PGconn and call
PQsprinkleMagicCancelDust() on it, and then you just proceed using the
existing functions, or something like that. Then, not only do the
existing functions not need query-cancel analogues, but any new
functions we add in the future don't either.
I'll set the target version for this patch to 16. I hope work continues.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-25 18:46 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-03-25 18:46 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Jacob Champion <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> That said, I don't think that this particular patch is going in the
> right direction. I think Jacob's comment upthread is right on point:
> "This seems like a big change compared to PQcancel(); one that's not
> really hinted at elsewhere. Having the async version of an API open up
> a completely different code path with new features is pretty
> surprising to me." It seems to me that we want to end up with similar
> code paths for PQcancel() and the non-blocking version of cancel. We
> could get there in two ways. One way would be to implement the
> non-blocking functionality in a manner that matches exactly what
> PQcancel() does now. I imagine that the existing code from PQcancel()
> would move, with some amount of change, into a new set of non-blocking
> APIs. Perhaps PQcancel() would then be rewritten to use those new APIs
> instead of hand-rolling the same logic. The other possible approach
> would be to first change the blocking version of PQcancel() to use the
> regular connection code instead of its own idiosyncratic logic, and
> then as a second step, extend it with non-blocking interfaces that use
> the regular non-blocking connection code. With either of these
> approaches, we end up with the functionality working similarly in the
> blocking and non-blocking code paths.
I think you misunderstand where the real pain point is. The reason
that PQcancel's functionality is so limited has little to do with
blocking vs non-blocking, and everything to do with the fact that
it's designed to be safe to call from a SIGINT handler. That makes
it quite impractical to invoke OpenSSL, and probably our GSS code
as well. If we want support for all connection-time options then
we have to make a new function that does not promise signal safety.
I'm prepared to yield on the question of whether we should provide
a non-blocking version, though I still say that (a) an easier-to-call,
one-step blocking alternative would be good too, and (b) it should
not be designed around the assumption that there's a completely
independent state object being used to perform the cancel. Even in
the non-blocking case, callers should only deal with the original
PGconn.
> Leaving the question of approach aside, I think it's fairly clear that
> this patch cannot be seriously considered for v15.
Yeah, I don't think it's anywhere near fully baked yet. On the other
hand, we do have a couple of weeks left.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-25 19:22 Robert Haas <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Haas @ 2022-03-25 19:22 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Jacob Champion <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
On Fri, Mar 25, 2022 at 2:47 PM Tom Lane <[email protected]> wrote:
> I think you misunderstand where the real pain point is. The reason
> that PQcancel's functionality is so limited has little to do with
> blocking vs non-blocking, and everything to do with the fact that
> it's designed to be safe to call from a SIGINT handler. That makes
> it quite impractical to invoke OpenSSL, and probably our GSS code
> as well. If we want support for all connection-time options then
> we have to make a new function that does not promise signal safety.
Well, that's a fair point, but it's somewhat orthogonal to the one I'm
making, which is that a non-blocking version of function X might be
expected to share code or at least functionality with X itself. Having
something that is named in a way that implies asynchrony without other
differences but which is actually different in other important ways is
no good.
> I'm prepared to yield on the question of whether we should provide
> a non-blocking version, though I still say that (a) an easier-to-call,
> one-step blocking alternative would be good too, and (b) it should
> not be designed around the assumption that there's a completely
> independent state object being used to perform the cancel. Even in
> the non-blocking case, callers should only deal with the original
> PGconn.
Well, this sounds like you're arguing for the first of the two
approaches I thought would be acceptable, rather than the second.
> > Leaving the question of approach aside, I think it's fairly clear that
> > this patch cannot be seriously considered for v15.
>
> Yeah, I don't think it's anywhere near fully baked yet. On the other
> hand, we do have a couple of weeks left.
We do?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-25 19:34 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2022-03-25 19:34 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Jacob Champion <[email protected]>; [email protected] <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> Well, that's a fair point, but it's somewhat orthogonal to the one I'm
> making, which is that a non-blocking version of function X might be
> expected to share code or at least functionality with X itself. Having
> something that is named in a way that implies asynchrony without other
> differences but which is actually different in other important ways is
> no good.
Yeah. We need to choose a name for these new function(s) that is
sufficiently different from "PQcancel" that people won't expect them
to behave exactly the same as that does. I lack any good ideas about
that, how about you?
>> Yeah, I don't think it's anywhere near fully baked yet. On the other
>> hand, we do have a couple of weeks left.
> We do?
Um, you did read the psql-release discussion about setting the feature
freeze deadline, no?
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
@ 2022-03-28 09:28 Jelte Fennema <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jelte Fennema @ 2022-03-28 09:28 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Jacob Champion <[email protected]>; PostgreSQL Hackers <[email protected]>
Thanks for all the feedback everyone. I'll try to send a new patch
later this week that includes user facing docs and a simplified API.
For now a few responses:
> Yeah. We need to choose a name for these new function(s) that is
> sufficiently different from "PQcancel" that people won't expect them
> to behave exactly the same as that does. I lack any good ideas about
> that, how about you?
So I guess the names I proposed were not great, since everyone seems to be falling over them.
But I'd like to make my intention clear with the current naming. After this patch there would be
four different APIs for starting a cancelation:
1. PQrequestCancel: deprecated+old, not signal-safe function for requesting query cancellation, only uses a specific set of connection options
2. PQcancel: Cancel queries in a signal safe way, to be signal-safe it only uses a limited set of connection options
3. PQcancelConnect: Cancel queries in a non-signal safe way that uses all connection options
4. PQcancelConnectStart: Cancel queries in a non-signal safe and non-blocking way that uses all connection options
So the idea was that you should not look at PQcancelConnectStart as the non-blocking
version of PQcancel, but as the non-blocking version of PQcancelConnect. I'll try to
think of some different names too, but IMHO these names could be acceptable
when their differences are addressed sufficiently in the documentation.
One other approach to naming that comes to mind now is repurposing PQrequestCancel:
1. PQrequestCancel: Cancel queries in a non-signal safe way that uses all connection options
2. PQrequestCancelStart: Cancel queries in a non-signal safe and non-blocking way that uses all connection options
3. PQcancel: Cancel queries in a signal safe way, to be signal-safe it only uses a limited set of connection options
> I think it's probably a
> bad sign that this function is tinkering with logic in this sort of
> low-level function anyway. pqReadData() is a really general function
> that manages to work with non-blocking I/O already, so why does
> non-blocking query cancellation need to change its return values, or
> whether or not it drops data in certain cases?
The reason for this low level change is that the cancellation part of the
Postgres protocol is following a different, much more simplistic design
than all the other parts. The client does not expect a response message back
from the server after sending the cancellation request. The expectation
is that the server signals completion by closing the connection, i.e. sending EOF.
For all other parts of the protocol, connection termination should be initiated
client side by sending a Terminate message. So the server closing (sending
EOF) is always unexpected and is thus currently considered an error by pqReadData.
But since this is not the case for the cancellation protocol, the result is
changed to -2 in case of EOF to make it possible to distinguish between
an EOF and an actual error.
> And it updates that function to return -2 when the is
> ECONNRESET, which seems to fly in the face of the comment's idea that
> this is the "clean connection closure" case.
The diff sadly does not include the very relevant comment right above these
lines. Pasting the whole case statement here to clear up this confusion:
case SSL_ERROR_ZERO_RETURN:
/*
* Per OpenSSL documentation, this error code is only returned for
* a clean connection closure, so we should not report it as a
* server crash.
*/
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("SSL connection has been closed unexpectedly\n"));
result_errno = ECONNRESET;
n = -2;
break;
> For example, it updates the
> comment for pqsecure_read() to say "Returns -1 in case of failures,
> except in the case of clean connection closure then it returns -2."
> But that function calls any of three different implementation
> functions depending on the situation and the patch only updates one of
> them.
That comment is indeed not describing what is happening correctly and I'll
try to make it clearer. The main reason for it being incorrect is coming from
the fact that receiving EOFs is handled in different places based on the
encryption method:
1. Unencrypted TCP: EOF is not returned as an error by pqsecure_read, but detected by pqReadData (see comments related to definitelyEOF)
2. OpenSSL: EOF is returned as an error by pqsecure_read (see copied case statement above)
3. GSS: When writing the patch I was not sure how EOF handling worked here, but given that the tests passed for Jacob on GSS, I'm guessing it works the same as unencrypted TCP.
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-30 16:08 Jelte Fennema <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jelte Fennema @ 2022-03-30 16:08 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Jacob Champion <[email protected]>; PostgreSQL Hackers <[email protected]>
I attached a new version of this patch. Which does three main things:
1. Change the PQrequestCancel implementation to use the regular
connection establishement code, to support all connection options
including encryption.
2. Add PQrequestCancelStart which is a thread-safe and non-blocking
version of this new PQrequestCancel implementation.
3. Add PQconnectComplete, which completes a connection started by
PQrequestCancelStart. This is useful if you want a thread-safe, but
blocking cancel (without having a need for signal safety).
This change un-deprecates PQrequestCancel, since now there's actually an
advantage to using it over PQcancel. It also includes user facing documentation
for all these functions.
As a API design change from the previous version, PQrequestCancelStart now
returns a regular PGconn for the cancel connection.
@Tom Lane regarding this:
> Even in the non-blocking case, callers should only deal with the original PGconn.
This would by definition result in non-threadsafe code (afaict). So I refrained from doing this.
The blocking version doesn't expose a PGconn at all, but the non-blocking one now returns a new PGconn.
There's two more changes that I at least want to do before considering this patch mergable:
1. Go over all the functions that can be called with a PGconn, but should not be
called with a cancellation PGconn and error out or exit early.
2. Copy over the SockAddr from the original connection and always connect to
the same socket. I believe with the current code the cancellation could end up
at the wrong server if there are multiple hosts listed in the connection string.
And there's a third item that I would like to do as a bonus:
3. Actually use the non-blocking API for the postgres_fdw code to implement a
timeout. Which would allow this comment can be removed:
/*
* Issue cancel request. Unfortunately, there's no good way to limit the
* amount of time that we might block inside PQgetCancel().
*/
So a next version of this patch can be expected somewhere later this week.
But any feedback on the current version would be appreciated. Because
these 3 changes won't change the overall design much.
Jelte
Attachments:
[application/octet-stream] 0001-Add-documentation-for-libpq_pipeline-tests.patch (1.5K, ../../HE1PR8303MB00735DC38F45BD63A3A49347F71F9@HE1PR8303MB0073.EURPRD83.prod.outlook.com/2-0001-Add-documentation-for-libpq_pipeline-tests.patch)
download | inline diff:
From 22a02899d47d46ed05ada2e38e3f9804981b96eb Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Thu, 13 Jan 2022 15:26:35 +0100
Subject: [PATCH 1/2] Add documentation for libpq_pipeline tests
This adds some explanation on how to run and add libpq tests.
---
src/test/modules/libpq_pipeline/README | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/test/modules/libpq_pipeline/README b/src/test/modules/libpq_pipeline/README
index d8174dd579..6eda6c5756 100644
--- a/src/test/modules/libpq_pipeline/README
+++ b/src/test/modules/libpq_pipeline/README
@@ -1 +1,21 @@
Test programs and libraries for libpq
+=====================================
+
+You can manually run a specific test by running:
+
+ ./libpq_pipeline <name of test>
+
+To add a new libpq test to this module you need to edit libpq_pipeline.c. There
+you should add the name of your new test to the "print_test_list" function.
+Then in main you should do something when this test name is passed to the
+program.
+
+If the order in which postgres protocol messages are sent is deterministic for
+your test. Then you can generate a trace of these messages using the following
+command:
+
+ ./libpq_pipeline mynewtest -t traces/mynewtest.trace
+
+Once you've done that you should make sure that when running "make check"
+the generated trace is compared to the expected trace. This is done by adding
+your test name to the $cmptrace definition in the t/001_libpq_pipeline.pl file
--
2.17.1
[application/octet-stream] 0002-Add-non-blocking-version-of-PQcancel.patch (39.0K, ../../HE1PR8303MB00735DC38F45BD63A3A49347F71F9@HE1PR8303MB0073.EURPRD83.prod.outlook.com/3-0002-Add-non-blocking-version-of-PQcancel.patch)
download | inline diff:
From 98a67a65eee6b7ee1275e48e5053ba8ab3055014 Mon Sep 17 00:00:00 2001
From: Jelte Fennema <[email protected]>
Date: Wed, 12 Jan 2022 09:52:05 +0100
Subject: [PATCH 2/2] Add non-blocking version of PQcancel
This patch does two things:
1. Change PQrequestCancel to use the regular connection establishement,
to address a few security issues.
2. Add PQrequestCancelStart which is a thread safe and non blocking
version of this new PQrequestCancel implementation.
The existing PQcancel API is using blocking IO. This makes PQcancel
impossible to use in an event loop based codebase, without blocking the
event loop until the call returns.
This patch adds a new cancellation API to libpq which is called
PQrequestCancelStart. This API can be used to send cancellations in a
non-blocking fashion.
This patch also includes a test for this all of libpq cancellation APIs.
The test can be easily run like this:
cd src/test/modules/libpq_pipeline
make && ./libpq_pipeline cancel
---
contrib/dblink/dblink.c | 12 +-
contrib/postgres_fdw/connection.c | 11 +-
doc/src/sgml/libpq.sgml | 212 +++++++++++++----
src/fe_utils/connect_utils.c | 10 +-
src/interfaces/libpq/exports.txt | 2 +
src/interfaces/libpq/fe-connect.c | 225 +++++++++++++++---
src/interfaces/libpq/fe-misc.c | 15 +-
src/interfaces/libpq/fe-secure-openssl.c | 2 +-
src/interfaces/libpq/fe-secure.c | 6 +
src/interfaces/libpq/libpq-fe.h | 13 +-
src/interfaces/libpq/libpq-int.h | 2 +
src/test/isolation/isolationtester.c | 29 +--
.../modules/libpq_pipeline/libpq_pipeline.c | 214 ++++++++++++++++-
13 files changed, 627 insertions(+), 126 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index a06d4bd12d..30cbb22a22 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -1380,22 +1380,14 @@ PG_FUNCTION_INFO_V1(dblink_cancel_query);
Datum
dblink_cancel_query(PG_FUNCTION_ARGS)
{
- int res;
PGconn *conn;
- PGcancel *cancel;
- char errbuf[256];
dblink_init();
conn = dblink_get_named_conn(text_to_cstring(PG_GETARG_TEXT_PP(0)));
- cancel = PQgetCancel(conn);
-
- res = PQcancel(cancel, errbuf, 256);
- PQfreeCancel(cancel);
-
- if (res == 1)
+ if (PQrequestCancel(conn))
PG_RETURN_TEXT_P(cstring_to_text("OK"));
else
- PG_RETURN_TEXT_P(cstring_to_text(errbuf));
+ PG_RETURN_TEXT_P(cstring_to_text(PQerrorMessage(conn)));
}
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 129ca79221..2e182645f7 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -1263,8 +1263,6 @@ pgfdw_reset_xact_state(ConnCacheEntry *entry, bool toplevel)
static bool
pgfdw_cancel_query(PGconn *conn)
{
- PGcancel *cancel;
- char errbuf[256];
PGresult *result = NULL;
TimestampTz endtime;
bool timed_out;
@@ -1279,19 +1277,14 @@ pgfdw_cancel_query(PGconn *conn)
* Issue cancel request. Unfortunately, there's no good way to limit the
* amount of time that we might block inside PQgetCancel().
*/
- if ((cancel = PQgetCancel(conn)))
- {
- if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ if (!PQrequestCancel(conn))
{
ereport(WARNING,
(errcode(ERRCODE_CONNECTION_FAILURE),
errmsg("could not send cancel request: %s",
- errbuf)));
- PQfreeCancel(cancel);
+ pchomp(PQerrorMessage(conn)))));
return false;
}
- PQfreeCancel(cancel);
- }
/* Get and discard the result of the query. */
if (pgfdw_get_cleanup_result(conn, endtime, &result, &timed_out))
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0b2a8720f0..6b0683b9b0 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -265,7 +265,7 @@ PGconn *PQsetdb(char *pghost,
<varlistentry id="libpq-PQconnectStartParams">
<term><function>PQconnectStartParams</function><indexterm><primary>PQconnectStartParams</primary></indexterm></term>
<term><function>PQconnectStart</function><indexterm><primary>PQconnectStart</primary></indexterm></term>
- <term><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
+ <term id="libpq-PQconnectPoll"><function>PQconnectPoll</function><indexterm><primary>PQconnectPoll</primary></indexterm></term>
<listitem>
<para>
<indexterm><primary>nonblocking connection</primary></indexterm>
@@ -499,6 +499,30 @@ switch(PQstatus(conn))
</listitem>
</varlistentry>
+ <varlistentry id="libpq-PQconnectComplete">
+ <term><function>PQconnectComplete</function><indexterm><primary>PQconnectComplete</primary></indexterm></term>
+ <listitem>
+ <para>
+ Complete the connection attempt on a nonblocking connection and block
+ until it is completed.
+
+<synopsis>
+int PQconnectPoll(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ This function can be used instead of
+ <xref linkend="libpq-PQconnectPoll"/>
+ to complete a connection that was initially started in a non blocking
+ manner. However, instead of continuing to complete the connection in a
+ non blocking way, calling this function will block until the connection
+ is completed. This is especially useful to complete connections that were
+ started by <xref linkend="libpq-PQrequestCancelStart"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-PQconndefaults">
<term><function>PQconndefaults</function><indexterm><primary>PQconndefaults</primary></indexterm></term>
<listitem>
@@ -660,7 +684,7 @@ void PQreset(PGconn *conn);
<varlistentry id="libpq-PQresetStart">
<term><function>PQresetStart</function><indexterm><primary>PQresetStart</primary></indexterm></term>
- <term><function>PQresetPoll</function><indexterm><primary>PQresetPoll</primary></indexterm></term>
+ <term id="libpq-PQresetPoll"><function>PQresetPoll</function><indexterm><primary>PQresetPoll</primary></indexterm></term>
<listitem>
<para>
Reset the communication channel to the server, in a nonblocking manner.
@@ -5617,13 +5641,137 @@ int PQsetSingleRowMode(PGconn *conn);
this section.
<variablelist>
+ <varlistentry id="libpq-PQrequestCancel">
+ <term><function>PQrequestCancel</function><indexterm><primary>PQrequestCancel</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ Requests that the server abandons processing of the current command.
+<synopsis>
+int PQrequestCancel(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ This request is made over a connection that uses the same connection
+ options as the the original <structname>PGconn</structname>. So when the
+ original connection is encrypted (using TLS or GSS), the connection for
+ the cancel request connection is encrypted in the same. Any connection
+ options that only make sense for authentication or after authentication
+ are ignored though, because cancellation requests do not require
+ authentication.
+ </para>
+
+ <para>
+ This function operates directly on the <structname>PGconn</structname>
+ object, and in case of failure stores the error message in the
+ <structname>PGconn</structname> object (whence it can be retrieved
+ by <xref linkend="libpq-PQerrorMessage"/>). This behaviour makes this
+ function unsafe to call from within multi-threaded programs or
+ signal handlers, since it is possible that overwriting the
+ <structname>PGconn</structname>'s error message will
+ mess up the operation currently in progress on the connection in another
+ thread.
+ </para>
+
+ <para>
+ The return value is 1 if the cancel request was successfully
+ dispatched and 0 if not. Successful dispatch is no guarantee that the
+ request will have any effect, however. If the cancellation is effective,
+ the current command will terminate early and return an error result. If
+ the cancellation fails (say, because the server was already done
+ processing the command), then there will be no visible result at
+ all.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-PQrequestCancelStart">
+ <term><function>PQrequestCancelStart</function><indexterm><primary>PQrequestCancelStart</primary></indexterm></term>
+
+ <listitem>
+ <para>
+ A version of
+ <xref linkend="libpq-PQrequestCancel"/>
+ that can be used in thread-safe and/or non-blocking manner.
+<synopsis>
+PGconn *PQrequestCancelStart(PGconn *conn);
+</synopsis>
+ </para>
+
+ <para>
+ This function returns a new <structname>PGconn</structname>. This
+ connection object can be used to cancel the query that's running on the
+ original connection in a thread-safe way. To do so
+ <xref linkend="libpq-PQrequestCancel"/>
+ must be called while no other thread is using the original PGconn. Then
+ the returned <structname>PGconn</structname>
+ can be used at a later point in any thread to send a cancel request.
+ A cancel request can be sent using the returned PGconn in two ways,
+ non-blocking using <xref linkend="libpq-PQconnectPoll"/>
+ or blocking using <xref linkend="libpq-PQconnectComplete"/>.
+ </para>
+
+ <para>
+ In addition to all the statuses that a regular
+ <structname>PGconn</structname>
+ can have returned connection can have two additional statuses:
+
+ <variablelist>
+ <varlistentry id="libpq-connection-starting">
+ <term><symbol>CONNECTION_STARTING</symbol></term>
+ <listitem>
+ <para>
+ Waiting for the first call to <xref linkend="libpq-PQconnectPoll"/>,
+ to actually open the socket. This is the connection state right after
+ calling <xref linkend="libpq-PQrequestCancel"/>. No connection to the
+ server has been initiated yet at this point. To start cancel request
+ initiation use <xref linkend="libpq-PQconnectPoll"/>
+ for non-blocking behaviour and <xref linkend="libpq-PQconnectComplete"/>
+ for blocking behaviour.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-connection-cancel-finished">
+ <term><symbol>CONNECTION_CANCEL_FINISHED</symbol></term>
+ <listitem>
+ <para>
+ Cancel request was successfully sent. It's not possible to continue
+ using the cancellation connection now, so it should be freed using
+ <xref linkend="libpq-PQfinish"/>. It's also possible to reset the
+ cancellation connection instead using
+ <xref linkend="libpq-PQresetStart"/>, that way it can be reused to
+ cancel a future query on the same connection.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+
+ <para>
+ Since this object represents a connection only meant for cancellations it
+ can only be used with a limited subset of the functions that can be used
+ for a regular <structname>PGconn</structname> object. The functions that
+ this object can be passed to are
+ <xref linkend="libpq-PQstatus"/>,
+ <xref linkend="libpq-PQerrorMessage"/>,
+ <xref linkend="libpq-PQconnectComplete"/>,
+ <xref linkend="libpq-PQconnectPoll"/>,
+ <xref linkend="libpq-PQsocket"/>,
+ <xref linkend="libpq-PQresetStart"/>, and
+ <xref linkend="libpq-PQfinish"/>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-PQgetCancel">
<term><function>PQgetCancel</function><indexterm><primary>PQgetCancel</primary></indexterm></term>
<listitem>
<para>
Creates a data structure containing the information needed to cancel
- a command issued through a particular database connection.
+ a command using <xref linkend="libpq-PQcancel"/>.
<synopsis>
PGcancel *PQgetCancel(PGconn *conn);
</synopsis>
@@ -5665,7 +5813,9 @@ void PQfreeCancel(PGcancel *cancel);
<listitem>
<para>
- Requests that the server abandon processing of the current command.
+ A less secure version of
+ <xref linkend="libpq-PQrequestCancel"/>
+ that can be used safely from within a signal handler.
<synopsis>
int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
</synopsis>
@@ -5679,15 +5829,6 @@ int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
recommended size is 256 bytes).
</para>
- <para>
- Successful dispatch is no guarantee that the request will have
- any effect, however. If the cancellation is effective, the current
- command will terminate early and return an error result. If the
- cancellation fails (say, because the server was already done
- processing the command), then there will be no visible result at
- all.
- </para>
-
<para>
<xref linkend="libpq-PQcancel"/> can safely be invoked from a signal
handler, if the <parameter>errbuf</parameter> is a local variable in the
@@ -5696,33 +5837,24 @@ int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
also be invoked from a thread that is separate from the one
manipulating the <structname>PGconn</structname> object.
</para>
- </listitem>
- </varlistentry>
- </variablelist>
-
- <variablelist>
- <varlistentry id="libpq-PQrequestCancel">
- <term><function>PQrequestCancel</function><indexterm><primary>PQrequestCancel</primary></indexterm></term>
-
- <listitem>
- <para>
- <xref linkend="libpq-PQrequestCancel"/> is a deprecated variant of
- <xref linkend="libpq-PQcancel"/>.
-<synopsis>
-int PQrequestCancel(PGconn *conn);
-</synopsis>
- </para>
<para>
- Requests that the server abandon processing of the current
- command. It operates directly on the
- <structname>PGconn</structname> object, and in case of failure stores the
- error message in the <structname>PGconn</structname> object (whence it can
- be retrieved by <xref linkend="libpq-PQerrorMessage"/>). Although
- the functionality is the same, this approach is not safe within
- multiple-thread programs or signal handlers, since it is possible
- that overwriting the <structname>PGconn</structname>'s error message will
- mess up the operation currently in progress on the connection.
+ To achieve signal-safety, some concessions needed to be made in the
+ implementation of <xref linkend="libpq-PQcancel"/>. Not all connection
+ options of the original connection are used when establishing a
+ connection for the cancellation request. When calling this function a
+ connection is made to the postgres host using the same port. The only
+ connection options that are honored during this connection are
+ <varname>keepalives</varname>,
+ <varname>keepalives_idle</varname>,
+ <varname>keepalives_interval</varname>,
+ <varname>keepalives_count</varname>, and
+ <varname>tcp_user_timeout</varname>.
+ So, for example
+ <varname>connect_timeout</varname>,
+ <varname>gssencmode</varname>, and
+ <varname>sslmode</varname> are ignored. This means the connection
+ is never encrypted using TLS or GSS.
</para>
</listitem>
</varlistentry>
@@ -8835,10 +8967,10 @@ int PQisthreadsafe();
</para>
<para>
- The deprecated functions <xref linkend="libpq-PQrequestCancel"/> and
+ The functions <xref linkend="libpq-PQrequestCancel"/> and
<xref linkend="libpq-PQoidStatus"/> are not thread-safe and should not be
used in multithread programs. <xref linkend="libpq-PQrequestCancel"/>
- can be replaced by <xref linkend="libpq-PQcancel"/>.
+ can be replaced by <xref linkend="libpq-PQrequestCancelStart"/>.
<xref linkend="libpq-PQoidStatus"/> can be replaced by
<xref linkend="libpq-PQoidValue"/>.
</para>
diff --git a/src/fe_utils/connect_utils.c b/src/fe_utils/connect_utils.c
index a30c66f13a..ff18dab043 100644
--- a/src/fe_utils/connect_utils.c
+++ b/src/fe_utils/connect_utils.c
@@ -162,19 +162,11 @@ connectMaintenanceDatabase(ConnParams *cparams,
void
disconnectDatabase(PGconn *conn)
{
- char errbuf[256];
-
Assert(conn != NULL);
if (PQtransactionStatus(conn) == PQTRANS_ACTIVE)
{
- PGcancel *cancel;
-
- if ((cancel = PQgetCancel(conn)))
- {
- (void) PQcancel(cancel, errbuf, sizeof(errbuf));
- PQfreeCancel(cancel);
- }
+ (void) PQrequestCancel(conn);
}
PQfinish(conn);
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index e8bcc88370..f7609d0c64 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -186,3 +186,5 @@ PQpipelineStatus 183
PQsetTraceFlags 184
PQmblenBounded 185
PQsendFlushRequest 186
+PQrequestCancelStart 187
+PQconnectComplete 188
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index cf554d389f..5462e1305c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -378,6 +378,7 @@ static int connectDBComplete(PGconn *conn);
static PGPing internal_ping(PGconn *conn);
static PGconn *makeEmptyPGconn(void);
static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
+static bool copyPGconn(PGconn *srcConn, PGconn *dstConn);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
static void release_conn_addrinfo(PGconn *conn);
@@ -604,8 +605,11 @@ pqDropServerData(PGconn *conn)
if (conn->write_err_msg)
free(conn->write_err_msg);
conn->write_err_msg = NULL;
- conn->be_pid = 0;
- conn->be_key = 0;
+ if (!conn->cancelRequest)
+ {
+ conn->be_pid = 0;
+ conn->be_key = 0;
+ }
}
@@ -737,6 +741,58 @@ PQping(const char *conninfo)
return ret;
}
+/*
+ * PQcancelConnectStart
+ *
+ * Asynchronously cancel a request on the given connection. This requires
+ * polling the returned PGconn to actually complete the cancellation of the
+ * request.
+ */
+PGconn *
+PQrequestCancelStart(PGconn *conn)
+{
+ PGconn *cancelConn = makeEmptyPGconn();
+
+ if (cancelConn == NULL)
+ return NULL;
+
+ /* Check we have an open connection */
+ if (!conn)
+ {
+ appendPQExpBufferStr(&cancelConn->errorMessage, libpq_gettext("passed connection was NULL\n"));
+ return cancelConn;
+ }
+
+ if (conn->sock == PGINVALID_SOCKET)
+ {
+ appendPQExpBufferStr(&cancelConn->errorMessage, libpq_gettext("passed connection is not open\n"));
+ return cancelConn;
+ }
+
+ /*
+ * Indicate that this connection is used to send a cancellation
+ */
+ cancelConn->cancelRequest = true;
+
+ if (!copyPGconn(conn, cancelConn))
+ return (PGconn *) cancelConn;
+
+ /*
+ * Copy over information needed to cancel
+ */
+ cancelConn->be_pid = conn->be_pid;
+ cancelConn->be_key = conn->be_key;
+
+ /*
+ * Compute derived options
+ */
+ if (!connectOptions2(cancelConn))
+ return cancelConn;
+
+ cancelConn->status = CONNECTION_STARTING;
+ return cancelConn;
+}
+
/*
* PQconnectStartParams
*
@@ -914,6 +970,46 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
return true;
}
+/*
+ * Copy over option values from srcConn to dstConn
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message of
+ * dstConn.
+ */
+static bool
+copyPGconn(PGconn *srcConn, PGconn *dstConn)
+{
+ const internalPQconninfoOption *option;
+
+ /* copy over connection options */
+ for (option = PQconninfoOptions; option->keyword; option++)
+ {
+ if (option->connofs >= 0)
+ {
+ const char **tmp = (const char **) ((char *) srcConn + option->connofs);
+
+ if (*tmp)
+ {
+ char **dstConnmember = (char **) ((char *) dstConn + option->connofs);
+
+ if (*dstConnmember)
+ free(*dstConnmember);
+ *dstConnmember = strdup(*tmp);
+ if (*dstConnmember == NULL)
+ {
+ appendPQExpBufferStr(&dstConn->errorMessage,
+ libpq_gettext("out of memory\n"));
+ return false;
+ }
+ }
+ }
+ }
+ return true;
+}
+
/*
* connectOptions1
*
@@ -2134,6 +2230,15 @@ connectDBComplete(PGconn *conn)
if (conn == NULL || conn->status == CONNECTION_BAD)
return 0;
+ if (conn->status == CONNECTION_STARTING)
+ {
+ if (!connectDBStart(conn))
+ {
+ conn->status = CONNECTION_BAD;
+ return 0;
+ }
+ }
+
/*
* Set up a time limit, if connect_timeout isn't zero.
*/
@@ -2274,13 +2379,15 @@ PQconnectPoll(PGconn *conn)
switch (conn->status)
{
/*
- * We really shouldn't have been polled in these two cases, but we
- * can handle it.
+ * We really shouldn't have been polled in these three cases, but
+ * we can handle it.
*/
case CONNECTION_BAD:
return PGRES_POLLING_FAILED;
case CONNECTION_OK:
return PGRES_POLLING_OK;
+ case CONNECTION_CANCEL_FINISHED:
+ return PGRES_POLLING_OK;
/* These are reading states */
case CONNECTION_AWAITING_RESPONSE:
@@ -2292,6 +2399,17 @@ PQconnectPoll(PGconn *conn)
/* Load waiting data */
int n = pqReadData(conn);
+ if (n == -2 && conn->cancelRequest)
+ {
+ /*
+ * This is the expected end state for cancel connections.
+ * They are closed once the cancel is processed by the
+ * server.
+ */
+ conn->status = CONNECTION_CANCEL_FINISHED;
+ resetPQExpBuffer(&conn->errorMessage);
+ return PGRES_POLLING_OK;
+ }
if (n < 0)
goto error_return;
if (n == 0)
@@ -2301,6 +2419,7 @@ PQconnectPoll(PGconn *conn)
}
/* These are writing states, so we just proceed. */
+ case CONNECTION_STARTING:
case CONNECTION_STARTED:
case CONNECTION_MADE:
break;
@@ -2758,6 +2877,16 @@ keep_going: /* We will come back to here until there is
}
}
+ case CONNECTION_STARTING:
+ {
+ if (!connectDBStart(conn))
+ {
+ goto error_return;
+ }
+ conn->status = CONNECTION_STARTED;
+ return PGRES_POLLING_WRITING;
+ }
+
case CONNECTION_STARTED:
{
socklen_t optlen = sizeof(optval);
@@ -2966,6 +3095,25 @@ keep_going: /* We will come back to here until there is
}
#endif /* USE_SSL */
+ if (conn->cancelRequest)
+ {
+ CancelRequestPacket cancelpacket;
+
+ packetlen = sizeof(cancelpacket);
+ cancelpacket.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
+ cancelpacket.backendPID = pg_hton32(conn->be_pid);
+ cancelpacket.cancelAuthCode = pg_hton32(conn->be_key);
+ if (pqPacketSend(conn, 0, &cancelpacket, packetlen) != STATUS_OK)
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not send cancel packet: %s\n"),
+ SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
+ goto error_return;
+ }
+ conn->status = CONNECTION_AWAITING_RESPONSE;
+ return PGRES_POLLING_READING;
+ }
+
/*
* Build the startup packet.
*/
@@ -4194,6 +4342,11 @@ release_conn_addrinfo(PGconn *conn)
static void
sendTerminateConn(PGconn *conn)
{
+ if (conn->cancelRequest)
+ {
+ return;
+ }
+
/*
* Note that the protocol doesn't allow us to send Terminate messages
* during the startup phase.
@@ -4311,6 +4464,12 @@ PQresetStart(PGconn *conn)
{
closePGconn(conn);
+ if (conn->cancelRequest)
+ {
+ conn->status = CONNECTION_STARTING;
+ return 1;
+ }
+
return connectDBStart(conn);
}
@@ -4663,6 +4822,22 @@ cancel_errReturn:
return false;
}
+/*
+ * PQconnectComplete: takes a non blocking cancel connection and completes it
+ * in a blocking manner.
+ *
+ * Returns 1 if able to connect successfully and 0 if not.
+ *
+ * This can useful if you only care about the thread safety of
+ * PQrequestCancelStart and not about its non blocking functionality.
+ */
+int
+PQconnectComplete(PGconn *cancelConn)
+{
+ connectDBComplete(cancelConn);
+ return cancelConn->status != CONNECTION_BAD;
+}
+
/*
* PQrequestCancel: old, not thread-safe function for requesting query cancel
@@ -4679,45 +4854,31 @@ cancel_errReturn:
int
PQrequestCancel(PGconn *conn)
{
- int r;
- PGcancel *cancel;
+ PGconn *cancelConn = NULL;
- /* Check we have an open connection */
- if (!conn)
- return false;
-
- if (conn->sock == PGINVALID_SOCKET)
+ cancelConn = PQrequestCancelStart(conn);
+ if (!cancelConn)
{
- strlcpy(conn->errorMessage.data,
- "PQrequestCancel() -- connection is not open\n",
- conn->errorMessage.maxlen);
- conn->errorMessage.len = strlen(conn->errorMessage.data);
- conn->errorReported = 0;
-
+ appendPQExpBufferStr(&conn->errorMessage, libpq_gettext("out of memory\n"));
return false;
}
- cancel = PQgetCancel(conn);
- if (cancel)
- {
- r = PQcancel(cancel, conn->errorMessage.data,
- conn->errorMessage.maxlen);
- PQfreeCancel(cancel);
- }
- else
+ if (cancelConn->status == CONNECTION_BAD)
{
- strlcpy(conn->errorMessage.data, "out of memory",
- conn->errorMessage.maxlen);
- r = false;
+ appendPQExpBufferStr(&conn->errorMessage, PQerrorMessage(cancelConn));
+ freePGconn(cancelConn);
+ return false;
}
- if (!r)
+ if (!PQconnectComplete(cancelConn))
{
- conn->errorMessage.len = strlen(conn->errorMessage.data);
- conn->errorReported = 0;
+ appendPQExpBufferStr(&conn->errorMessage, PQerrorMessage(cancelConn));
+ freePGconn(cancelConn);
+ return false;
}
- return r;
+ freePGconn(cancelConn);
+ return true;
}
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index d76bb3957a..a944cb2c12 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -558,8 +558,11 @@ pqPutMsgEnd(PGconn *conn)
* Possible return values:
* 1: successfully loaded at least one more byte
* 0: no data is presently available, but no error detected
- * -1: error detected (including EOF = connection closure);
+ * -1: error detected (excluding EOF = connection closure);
* conn->errorMessage set
+ * -2: EOF detected, connection is closed
+ * conn->errorMessage set
+ *
* NOTE: callers must not assume that pointers or indexes into conn->inBuffer
* remain valid across this call!
* ----------
@@ -642,7 +645,7 @@ retry3:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -737,7 +740,7 @@ retry4:
default:
/* pqsecure_read set the error message for us */
- return -1;
+ return nread;
}
}
if (nread > 0)
@@ -755,13 +758,17 @@ definitelyEOF:
libpq_gettext("server closed the connection unexpectedly\n"
"\tThis probably means the server terminated abnormally\n"
"\tbefore or while processing the request.\n"));
+ /* Do *not* drop any already-read data; caller still wants it */
+ pqDropConnection(conn, false);
+ conn->status = CONNECTION_BAD; /* No more connection to backend */
+ return -2;
/* Come here if lower-level code already set a suitable errorMessage */
definitelyFailed:
/* Do *not* drop any already-read data; caller still wants it */
pqDropConnection(conn, false);
conn->status = CONNECTION_BAD; /* No more connection to backend */
- return -1;
+ return nread < 0 ? nread : -1;
}
/*
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index d3bf57b850..4ffaea63c1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -252,7 +252,7 @@ rloop:
appendPQExpBufferStr(&conn->errorMessage,
libpq_gettext("SSL connection has been closed unexpectedly\n"));
result_errno = ECONNRESET;
- n = -1;
+ n = -2;
break;
default:
appendPQExpBuffer(&conn->errorMessage,
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index a1dc7b796d..9771805dd3 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -201,6 +201,12 @@ pqsecure_close(PGconn *conn)
* On failure, this function is responsible for appending a suitable message
* to conn->errorMessage. The caller must still inspect errno, but only
* to determine whether to continue/retry after error.
+ *
+ * Returns -1 in case of failures, except in the case of where a failure means
+ * that there was a clean connection closure, in those cases -2 is return.
+ * Currently only the TLS implementation of pqsecure_read ever returns -2. For
+ * the other implementations a clean connection closure is detected in
+ * pqReadData instead.
*/
ssize_t
pqsecure_read(PGconn *conn, void *ptr, size_t len)
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 7986445f1a..42367d4886 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -59,12 +59,15 @@ typedef enum
{
CONNECTION_OK,
CONNECTION_BAD,
+ CONNECTION_CANCEL_FINISHED,
/* Non-blocking mode only below here */
/*
* The existence of these should never be relied upon - they should only
* be used for user feedback or similar purposes.
*/
+ CONNECTION_STARTING, /* Waiting for connection attempt to be
+ * started. */
CONNECTION_STARTED, /* Waiting for connection to be made. */
CONNECTION_MADE, /* Connection OK; waiting to send. */
CONNECTION_AWAITING_RESPONSE, /* Waiting for a response from the
@@ -165,6 +168,10 @@ typedef enum
*/
typedef struct pg_conn PGconn;
+/* PGcancelConn encapsulates a cancel connection to the backend.
+ * The contents of this struct are not supposed to be known to applications.
+ */
+
/* PGresult encapsulates the result of a query (or more precisely, of a single
* SQL command --- a query string given to PQsendQuery can contain multiple
* commands and thus return multiple PGresult objects).
@@ -282,6 +289,7 @@ extern PGconn *PQconnectStart(const char *conninfo);
extern PGconn *PQconnectStartParams(const char *const *keywords,
const char *const *values, int expand_dbname);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
+extern int PQconnectComplete(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
@@ -330,9 +338,12 @@ extern void PQfreeCancel(PGcancel *cancel);
/* issue a cancel request */
extern int PQcancel(PGcancel *cancel, char *errbuf, int errbufsize);
-/* backwards compatible version of PQcancel; not thread-safe */
+/* more secure version of PQcancel */
extern int PQrequestCancel(PGconn *conn);
+/* non blocking and thread safe version of PQrequestCancel */
+extern PGconn *PQrequestCancelStart(PGconn *conn);
+
/* Accessor functions for PGconn objects */
extern char *PQdb(const PGconn *conn);
extern char *PQuser(const PGconn *conn);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index e0cee4b142..ff9555e263 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -394,6 +394,8 @@ struct pg_conn
char *ssl_max_protocol_version; /* maximum TLS protocol version */
char *target_session_attrs; /* desired session properties */
+ bool cancelRequest;
+
/* Optional file to write trace info to */
FILE *Pfdebug;
int traceFlags;
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 12179f2514..fe1ca168c8 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -948,26 +948,17 @@ try_complete_step(TestSpec *testspec, PermutationStep *pstep, int flags)
*/
if (td > max_step_wait && !canceled)
{
- PGcancel *cancel = PQgetCancel(conn);
-
- if (cancel != NULL)
- {
- char buf[256];
-
- if (PQcancel(cancel, buf, sizeof(buf)))
- {
- /*
- * print to stdout not stderr, as this should appear
- * in the test case's results
- */
- printf("isolationtester: canceling step %s after %d seconds\n",
- step->name, (int) (td / USECS_PER_SEC));
- canceled = true;
- }
- else
- fprintf(stderr, "PQcancel failed: %s\n", buf);
- PQfreeCancel(cancel);
+ if (PQrequestCancel(conn)) {
+ /*
+ * print to stdout not stderr, as this should appear
+ * in the test case's results
+ */
+ printf("isolationtester: canceling step %s after %d seconds\n",
+ step->name, (int) (td / USECS_PER_SEC));
+ canceled = true;
}
+ else
+ fprintf(stderr, "PQcancel failed: %s\n", PQerrorMessage(conn));
}
/*
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 0ff563f59a..95f1d5eb2f 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -86,6 +86,215 @@ pg_fatal_impl(int line, const char *fmt,...)
exit(1);
}
+static void
+confirm_query_cancelled(PGconn *conn)
+{
+ PGresult *res = NULL;
+
+ res = PQgetResult(conn);
+ if (res == NULL)
+ pg_fatal("PQgetResult returned null: %s",
+ PQerrorMessage(conn));
+ if (PQresultStatus(res) != PGRES_FATAL_ERROR)
+ pg_fatal("query did not fail when it was expected");
+ if (strcmp(PQresultErrorField(res, PG_DIAG_SQLSTATE), "57014") != 0)
+ pg_fatal("query failed with a different error than cancellation: %s", PQerrorMessage(conn));
+ PQclear(res);
+ while (PQisBusy(conn))
+ {
+ PQconsumeInput(conn);
+ }
+}
+
+static void
+test_cancel(PGconn *conn)
+{
+ PGcancel *cancel = NULL;
+ PGconn *cancelConn = NULL;
+ char errorbuf[256];
+
+ fprintf(stderr, "test cancellations... ");
+
+ if (PQsetnonblocking(conn, 1) != 0)
+ pg_fatal("failed to set nonblocking mode: %s", PQerrorMessage(conn));
+
+ /* test PQcancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancel = PQgetCancel(conn);
+ if (!PQcancel(cancel, errorbuf, sizeof(errorbuf)))
+ {
+ pg_fatal("failed to run PQcancel: %s", errorbuf);
+ };
+ confirm_query_cancelled(conn);
+
+ /* PGcancel object can be reused for the next query */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ if (!PQcancel(cancel, errorbuf, sizeof(errorbuf)))
+ {
+ pg_fatal("failed to run PQcancel: %s", errorbuf);
+ };
+ confirm_query_cancelled(conn);
+
+ PQfreeCancel(cancel);
+
+ /* test PQrequestCancel */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ if (!PQrequestCancel(conn))
+ pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
+ confirm_query_cancelled(conn);
+
+ /* test PQrequestCancelStart and then polling with PQcancelConnectPoll */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQrequestCancelStart(conn);
+ if (PQstatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQconnectPoll(cancelConn);
+ int sock = PQsocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQerrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQstatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQerrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+
+ /*
+ * test PQresetStart works on the cancel connection and it can be reused
+ * after
+ */
+ if (!PQresetStart(cancelConn))
+ {
+ pg_fatal("cancel connection reset failed: %s", PQerrorMessage(cancelConn));
+ }
+
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ if (PQstatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ while (true)
+ {
+ struct timeval tv;
+ fd_set input_mask;
+ fd_set output_mask;
+ PostgresPollingStatusType pollres = PQresetPoll(cancelConn);
+ int sock = PQsocket(cancelConn);
+
+ if (pollres == PGRES_POLLING_OK)
+ {
+ break;
+ }
+
+ FD_ZERO(&input_mask);
+ FD_ZERO(&output_mask);
+ switch (pollres)
+ {
+ case PGRES_POLLING_READING:
+ pg_debug("polling for reads\n");
+ FD_SET(sock, &input_mask);
+ break;
+ case PGRES_POLLING_WRITING:
+ pg_debug("polling for writes\n");
+ FD_SET(sock, &output_mask);
+ break;
+ default:
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ }
+
+ if (sock < 0)
+ pg_fatal("sock did not exist: %s", PQerrorMessage(cancelConn));
+
+ tv.tv_sec = 3;
+ tv.tv_usec = 0;
+
+ while (true)
+ {
+ if (select(sock + 1, &input_mask, &output_mask, NULL, &tv) < 0)
+ {
+ if (errno == EINTR)
+ continue;
+ pg_fatal("select() failed: %m");
+ }
+ break;
+ }
+ }
+ if (PQstatus(cancelConn) != CONNECTION_CANCEL_FINISHED)
+ pg_fatal("unexpected cancel connection status: %s", PQerrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+
+ PQfinish(cancelConn);
+
+ /* test PQconnectComplete */
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ cancelConn = PQrequestCancelStart(conn);
+ if (PQstatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ if (!PQconnectComplete(cancelConn))
+ pg_fatal("failed to send cancel: %s", PQerrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+
+ /* test PQconnectComplete with reset connection */
+ if (!PQresetStart(cancelConn))
+ {
+ pg_fatal("cancel connection reset failed: %s", PQerrorMessage(cancelConn));
+ }
+
+ if (PQsendQuery(conn, "SELECT pg_sleep(3)") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ if (PQstatus(cancelConn) == CONNECTION_BAD)
+ pg_fatal("bad cancel connection: %s", PQerrorMessage(cancelConn));
+ if (!PQconnectComplete(cancelConn))
+ pg_fatal("failed to send cancel: %s", PQerrorMessage(cancelConn));
+ confirm_query_cancelled(conn);
+ PQfinish(cancelConn);
+
+ fprintf(stderr, "ok\n");
+}
+
static void
test_disallowed_in_pipeline(PGconn *conn)
{
@@ -1545,6 +1754,7 @@ usage(const char *progname)
static void
print_test_list(void)
{
+ printf("cancel\n");
printf("disallowed_in_pipeline\n");
printf("multi_pipelines\n");
printf("nosync\n");
@@ -1642,7 +1852,9 @@ main(int argc, char **argv)
PQTRACE_SUPPRESS_TIMESTAMPS | PQTRACE_REGRESS_MODE);
}
- if (strcmp(testname, "disallowed_in_pipeline") == 0)
+ if (strcmp(testname, "cancel") == 0)
+ test_cancel(conn);
+ else if (strcmp(testname, "disallowed_in_pipeline") == 0)
test_disallowed_in_pipeline(conn);
else if (strcmp(testname, "multi_pipelines") == 0)
test_multi_pipelines(conn);
--
2.17.1
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add non-blocking version of PQcancel
@ 2022-03-31 05:47 Justin Pryzby <[email protected]>
parent: Jelte Fennema <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Justin Pryzby @ 2022-03-31 05:47 UTC (permalink / raw)
To: Jelte Fennema <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Jacob Champion <[email protected]>; [email protected]
Note that the patch is still variously failing in cirrus.
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3511
You may already know that it's possible to trigger the cirrus ci tasks using a
github branch. See src/tools/ci/README.
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v16 2/8] Row pattern recognition patch (parse/analysis).
@ 2024-04-12 06:49 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2024-04-12 06:49 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 +
src/backend/parser/parse_clause.c | 296 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 4 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 309 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index bee7d8346a..9bc22a836a 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -577,6 +577,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -967,6 +971,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 4fc5fc87e0..003a1e14ce 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -98,7 +98,14 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
/*
* transformFromClause -
@@ -2956,6 +2963,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef, targetlist);
+
wc->runCondition = NIL;
wc->winref = winref;
@@ -3821,3 +3832,286 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /* Check Frame option. Frame must start at current row */
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+ /* Transform AFTER MACH SKIP TO variable */
+ wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable;
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ * list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /* DEFINE variable name initials */
+ static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+ ListCell *lc,
+ *l;
+ ResTarget *restarget,
+ *r;
+ List *restargets;
+ List *defineClause;
+ char *name;
+ int initialLen;
+ int i;
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist. (the
+ * raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+ * per the SQL standard.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * "name" is missing. So create "name AS name IS TRUE" ResTarget
+ * node and add it to the temporary list.
+ */
+ A_Const *n;
+
+ restarget = makeNode(ResTarget);
+ n = makeNode(A_Const);
+ n->val.boolval.type = T_Boolean;
+ n->val.boolval.boolval = true;
+ n->location = -1;
+ restarget->name = pstrdup(name);
+ restarget->indirection = NIL;
+ restarget->val = (Node *) n;
+ restarget->location = -1;
+ restargets = lappend((List *) restargets, restarget);
+ }
+ }
+
+ if (list_length(restargets) >= 1)
+ {
+ /* add missing DEFINEs */
+ windef->rpCommonSyntax->rpDefs =
+ list_concat(windef->rpCommonSyntax->rpDefs, restargets);
+ list_free(restargets);
+ }
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Add DEFINE expression (Restarget->val) to the targetlist as a
+ * TargetEntry if it does not exist yet. Planner will add the column
+ * ref var node to the outer plan's target list later on. This makes
+ * DEFINE expression could access the outer tuple while evaluating
+ * PATTERN.
+ *
+ * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+ * not so good, because it's not necessary to evalute the expression
+ * in the target list while running the plan. We should extract the
+ * var nodes only then add them to the plan.targetlist.
+ */
+ findTargetlistEntrySQL99(pstate, (Node *) restarget->val,
+ targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /*
+ * Make sure that the row pattern definition search condition is a
+ * boolean expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *) r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ /*
+ * Create list of row pattern DEFINE variable name's initial. We assign
+ * [a-z] to them (up to 26 variable names are allowed).
+ */
+ restargets = NIL;
+ i = 0;
+ initialLen = strlen(defineVariableInitials);
+
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char initial[2];
+
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ if (i >= initialLen)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("number of row pattern definition variable names exceeds %d",
+ initialLen),
+ parser_errposition(pstate,
+ exprLocation((Node *) restarget))));
+ }
+ initial[0] = defineVariableInitials[i++];
+ initial[1] = '\0';
+ wc->defineInitial = lappend(wc->defineInitial,
+ makeString(pstrdup(initial)));
+ }
+
+ defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+
+ /* mark column origins */
+ markTargetListOrigins(pstate, defineClause);
+
+ /* mark all nodes in the DEFINE clause tree with collation information */
+ assign_expr_collations(pstate, (Node *) defineClause);
+
+ return defineClause;
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ ListCell *lc;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ * XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s", "MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *) windef->rowPatternMeasures))));
+ return NIL;
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 4c98d7a046..a9f9d47854 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -578,6 +578,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1817,6 +1818,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_VALUES:
case EXPR_KIND_VALUES_SINGLE:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3197,6 +3199,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 0cbc950c95..ad982a7c17 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2657,6 +2657,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Fri_Apr_12_16_09_08_2024_262)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v16-0003-Row-pattern-recognition-patch-rewriter.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-04-12 06:49 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-12-19 20:32 [PATCH v18 16/18] WIP: Move xid horizon computation for page level index vacuum to primary. Andres Freund <[email protected]>
2022-01-12 15:22 Add non-blocking version of PQcancel Jelte Fennema <[email protected]>
2022-01-13 00:44 ` Re: Add non-blocking version of PQcancel Andres Freund <[email protected]>
2022-01-13 14:51 ` Re: Add non-blocking version of PQcancel Jelte Fennema <[email protected]>
2022-03-09 00:27 ` Re: Add non-blocking version of PQcancel Jacob Champion <[email protected]>
2022-03-24 21:41 ` Re: Add non-blocking version of PQcancel Tom Lane <[email protected]>
2022-03-24 22:49 ` Re: Add non-blocking version of PQcancel Andres Freund <[email protected]>
2022-03-25 18:34 ` Re: Add non-blocking version of PQcancel Robert Haas <[email protected]>
2022-03-25 18:46 ` Re: Add non-blocking version of PQcancel Tom Lane <[email protected]>
2022-03-25 19:22 ` Re: Add non-blocking version of PQcancel Robert Haas <[email protected]>
2022-03-25 19:34 ` Re: Add non-blocking version of PQcancel Tom Lane <[email protected]>
2022-03-28 09:28 ` Re: [EXTERNAL] Re: Add non-blocking version of PQcancel Jelte Fennema <[email protected]>
2022-03-30 16:08 ` Re: Add non-blocking version of PQcancel Jelte Fennema <[email protected]>
2022-03-31 05:47 ` Re: Add non-blocking version of PQcancel Justin Pryzby <[email protected]>
2024-04-12 06:49 [PATCH v16 2/8] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[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