public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE
6+ messages / 3 participants
[nested] [flat]
* [PATCH] Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE
@ 2021-01-10 19:30 Tomas Vondra <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Tomas Vondra @ 2021-01-10 19:30 UTC (permalink / raw)
Make sure COPY FREEZE marks the pages as PD_ALL_VISIBLE and updates the
visibility map. Until now it only marked individual tuples as frozen,
but page-level flags were not updated.
This is a fairly old patch, and multiple people worked on it. The first
version was written by Jeff Janes, and then reworked by Pavan Deolasee
and Anastasia Lubennikova.
Author: Pavan Deolasee, Anastasia Lubennikova, Jeff Janes
Reviewed-by: Kuntal Ghosh, Jeff Janes, Tomas Vondra, Masahiko Sawada, Andres Freund, Ibrar Ahmed, Robert Haas, Tatsuro Ishii
Discussion: https://postgr.es/m/CABOikdN-ptGv0mZntrK2Q8OtfUuAjqaYMGmkdU1dCKFtUxVLrg@mail.gmail.com
Discussion: https://postgr.es/m/CAMkU%3D1w3osJJ2FneELhhNRLxfZitDgp9FPHee08NT2FQFmz_pQ%40mail.gmail.com
---
.../pg_visibility/expected/pg_visibility.out | 64 +++++++++++++++
contrib/pg_visibility/sql/pg_visibility.sql | 77 +++++++++++++++++++
src/backend/access/heap/heapam.c | 76 ++++++++++++++++--
src/backend/access/heap/hio.c | 17 ++++
src/include/access/heapam_xlog.h | 3 +
5 files changed, 229 insertions(+), 8 deletions(-)
diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out
index ca4b6e186b..0017e3415c 100644
--- a/contrib/pg_visibility/expected/pg_visibility.out
+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -179,6 +179,69 @@ select pg_truncate_visibility_map('test_partition');
(1 row)
+-- test copy freeze
+create table copyfreeze (a int, b char(1500));
+-- load all rows via COPY FREEZE and ensure that all pages are set all-visible
+-- and all-frozen.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin freeze;
+commit;
+select * from pg_visibility_map('copyfreeze');
+ blkno | all_visible | all_frozen
+-------+-------------+------------
+ 0 | t | t
+ 1 | t | t
+ 2 | t | t
+(3 rows)
+
+select * from pg_check_frozen('copyfreeze');
+ t_ctid
+--------
+(0 rows)
+
+-- load half the rows via regular COPY and rest via COPY FREEZE. The pages
+-- which are touched by regular COPY must not be set all-visible/all-frozen. On
+-- the other hand, pages allocated by COPY FREEZE should be marked
+-- all-frozen/all-visible.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin;
+copy copyfreeze from stdin freeze;
+commit;
+select * from pg_visibility_map('copyfreeze');
+ blkno | all_visible | all_frozen
+-------+-------------+------------
+ 0 | f | f
+ 1 | f | f
+ 2 | t | t
+(3 rows)
+
+select * from pg_check_frozen('copyfreeze');
+ t_ctid
+--------
+(0 rows)
+
+-- Try a mix of regular COPY and COPY FREEZE.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin freeze;
+copy copyfreeze from stdin;
+copy copyfreeze from stdin freeze;
+commit;
+select * from pg_visibility_map('copyfreeze');
+ blkno | all_visible | all_frozen
+-------+-------------+------------
+ 0 | t | t
+ 1 | f | f
+ 2 | t | t
+(3 rows)
+
+select * from pg_check_frozen('copyfreeze');
+ t_ctid
+--------
+(0 rows)
+
-- cleanup
drop table test_partitioned;
drop view test_view;
@@ -188,3 +251,4 @@ drop server dummy_server;
drop foreign data wrapper dummy;
drop materialized view matview_visibility_test;
drop table regular_table;
+drop table copyfreeze;
diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql
index f79b54480b..ec1afd4906 100644
--- a/contrib/pg_visibility/sql/pg_visibility.sql
+++ b/contrib/pg_visibility/sql/pg_visibility.sql
@@ -94,6 +94,82 @@ select count(*) > 0 from pg_visibility_map_summary('test_partition');
select * from pg_check_frozen('test_partition'); -- hopefully none
select pg_truncate_visibility_map('test_partition');
+-- test copy freeze
+create table copyfreeze (a int, b char(1500));
+
+-- load all rows via COPY FREEZE and ensure that all pages are set all-visible
+-- and all-frozen.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin freeze;
+1 '1'
+2 '2'
+3 '3'
+4 '4'
+5 '5'
+6 '6'
+7 '7'
+8 '8'
+9 '9'
+10 '10'
+11 '11'
+12 '12'
+\.
+commit;
+select * from pg_visibility_map('copyfreeze');
+select * from pg_check_frozen('copyfreeze');
+
+-- load half the rows via regular COPY and rest via COPY FREEZE. The pages
+-- which are touched by regular COPY must not be set all-visible/all-frozen. On
+-- the other hand, pages allocated by COPY FREEZE should be marked
+-- all-frozen/all-visible.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin;
+1 '1'
+2 '2'
+3 '3'
+4 '4'
+5 '5'
+6 '6'
+\.
+copy copyfreeze from stdin freeze;
+7 '7'
+8 '8'
+9 '9'
+10 '10'
+11 '11'
+12 '12'
+\.
+commit;
+select * from pg_visibility_map('copyfreeze');
+select * from pg_check_frozen('copyfreeze');
+
+-- Try a mix of regular COPY and COPY FREEZE.
+begin;
+truncate copyfreeze;
+copy copyfreeze from stdin freeze;
+1 '1'
+2 '2'
+3 '3'
+4 '4'
+5 '5'
+\.
+copy copyfreeze from stdin;
+6 '6'
+\.
+copy copyfreeze from stdin freeze;
+7 '7'
+8 '8'
+9 '9'
+10 '10'
+11 '11'
+12 '12'
+\.
+commit;
+select * from pg_visibility_map('copyfreeze');
+select * from pg_check_frozen('copyfreeze');
+
-- cleanup
drop table test_partitioned;
drop view test_view;
@@ -103,3 +179,4 @@ drop server dummy_server;
drop foreign data wrapper dummy;
drop materialized view matview_visibility_test;
drop table regular_table;
+drop table copyfreeze;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 53e997cd55..32cc010cb7 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2103,6 +2103,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
int ndone;
PGAlignedBlock scratch;
Page page;
+ Buffer vmbuffer = InvalidBuffer;
bool needwal;
Size saveFreeSpace;
bool need_tuple_data = RelationIsLogicallyLogged(relation);
@@ -2157,8 +2158,9 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
while (ndone < ntuples)
{
Buffer buffer;
- Buffer vmbuffer = InvalidBuffer;
+ bool starting_with_empty_page;
bool all_visible_cleared = false;
+ bool all_frozen_set = false;
int nthispage;
CHECK_FOR_INTERRUPTS();
@@ -2166,12 +2168,20 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/*
* Find buffer where at least the next tuple will fit. If the page is
* all-visible, this will also pin the requisite visibility map page.
+ *
+ * Also pin visibility map page if COPY FREEZE inserts tuples into an
+ * empty page. See all_frozen_set below.
*/
buffer = RelationGetBufferForTuple(relation, heaptuples[ndone]->t_len,
InvalidBuffer, options, bistate,
&vmbuffer, NULL);
page = BufferGetPage(buffer);
+ starting_with_empty_page = PageGetMaxOffsetNumber(page) == 0;
+
+ if (starting_with_empty_page && (options & HEAP_INSERT_FROZEN))
+ all_frozen_set = true;
+
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
@@ -2205,7 +2215,14 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
log_heap_new_cid(relation, heaptup);
}
- if (PageIsAllVisible(page))
+ /*
+ * If the page is all visible, need to clear that, unless we're only
+ * going to add further frozen rows to it.
+ *
+ * If we're only adding already frozen rows to a previously empty
+ * page, mark it as all-visible.
+ */
+ if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN))
{
all_visible_cleared = true;
PageClearAllVisible(page);
@@ -2213,6 +2230,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
BufferGetBlockNumber(buffer),
vmbuffer, VISIBILITYMAP_VALID_BITS);
}
+ else if (all_frozen_set)
+ PageSetAllVisible(page);
/*
* XXX Should we set PageSetPrunable on this page ? See heap_insert()
@@ -2236,8 +2255,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
* If the page was previously empty, we can reinit the page
* instead of restoring the whole thing.
*/
- init = (ItemPointerGetOffsetNumber(&(heaptuples[ndone]->t_self)) == FirstOffsetNumber &&
- PageGetMaxOffsetNumber(page) == FirstOffsetNumber + nthispage - 1);
+ init = starting_with_empty_page;
/* allocate xl_heap_multi_insert struct from the scratch area */
xlrec = (xl_heap_multi_insert *) scratchptr;
@@ -2255,7 +2273,15 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/* the rest of the scratch space is used for tuple data */
tupledata = scratchptr;
- xlrec->flags = all_visible_cleared ? XLH_INSERT_ALL_VISIBLE_CLEARED : 0;
+ /* check that the mutually exclusive flags are not both set */
+ Assert (!(all_visible_cleared && all_frozen_set));
+
+ xlrec->flags = 0;
+ if (all_visible_cleared)
+ xlrec->flags = XLH_INSERT_ALL_VISIBLE_CLEARED;
+ if (all_frozen_set)
+ xlrec->flags = XLH_INSERT_ALL_FROZEN_SET;
+
xlrec->ntuples = nthispage;
/*
@@ -2329,13 +2355,39 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
END_CRIT_SECTION();
- UnlockReleaseBuffer(buffer);
- if (vmbuffer != InvalidBuffer)
- ReleaseBuffer(vmbuffer);
+ /*
+ * If we've frozen everything on the page, update the visibilitymap.
+ * We're already holding pin on the vmbuffer.
+ */
+ if (all_frozen_set)
+ {
+ Assert(PageIsAllVisible(page));
+ Assert(visibilitymap_pin_ok(BufferGetBlockNumber(buffer), vmbuffer));
+ /*
+ * It's fine to use InvalidTransactionId here - this is only used
+ * when HEAP_INSERT_FROZEN is specified, which intentionally
+ * violates visibility rules.
+ */
+ visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
+ InvalidXLogRecPtr, vmbuffer,
+ InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
+ }
+
+ UnlockReleaseBuffer(buffer);
ndone += nthispage;
+
+ /*
+ * NB: Only release vmbuffer after inserting all tuples - it's fairly
+ * likely that we'll insert into subsequent heap pages that are likely
+ * to use the same vm page.
+ */
}
+ if (vmbuffer != InvalidBuffer)
+ ReleaseBuffer(vmbuffer);
+
/*
* We're done with the actual inserts. Check for conflicts again, to
* ensure that all rw-conflicts in to these inserts are detected. Without
@@ -8265,6 +8317,10 @@ heap_xlog_multi_insert(XLogReaderState *record)
XLogRecGetBlockTag(record, 0, &rnode, NULL, &blkno);
+ /* check that the mutually exclusive flags are not both set */
+ Assert (!((xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) &&
+ (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET)));
+
/*
* The visibility map may need to be fixed even if the heap page is
* already up-to-date.
@@ -8354,6 +8410,10 @@ heap_xlog_multi_insert(XLogReaderState *record)
if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
PageClearAllVisible(page);
+ /* XLH_INSERT_ALL_FROZEN_SET implies that all tuples are visible */
+ if (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET)
+ PageSetAllVisible(page);
+
MarkBufferDirty(buffer);
}
if (BufferIsValid(buffer))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index fac3b8e9ff..2d23b3ef71 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -433,6 +433,14 @@ loop:
buffer = ReadBufferBI(relation, targetBlock, RBM_NORMAL, bistate);
if (PageIsAllVisible(BufferGetPage(buffer)))
visibilitymap_pin(relation, targetBlock, vmbuffer);
+
+ /*
+ * If the page is empty, pin vmbuffer to set all_frozen bit later.
+ */
+ if ((options & HEAP_INSERT_FROZEN) &&
+ (PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0))
+ visibilitymap_pin(relation, targetBlock, vmbuffer);
+
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
}
else if (otherBlock == targetBlock)
@@ -619,6 +627,15 @@ loop:
PageInit(page, BufferGetPageSize(buffer), 0);
MarkBufferDirty(buffer);
+ /*
+ * The page is empty, pin vmbuffer to set all_frozen bit.
+ */
+ if (options & HEAP_INSERT_FROZEN)
+ {
+ Assert(PageGetMaxOffsetNumber(BufferGetPage(buffer)) == 0);
+ visibilitymap_pin(relation, BufferGetBlockNumber(buffer), vmbuffer);
+ }
+
/*
* Release the file-extension lock; it's now OK for someone else to extend
* the relation some more.
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 51586b883d..178d49710a 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -69,6 +69,9 @@
#define XLH_INSERT_CONTAINS_NEW_TUPLE (1<<3)
#define XLH_INSERT_ON_TOAST_RELATION (1<<4)
+/* all_frozen_set always implies all_visible_set */
+#define XLH_INSERT_ALL_FROZEN_SET (1<<5)
+
/*
* xl_heap_update flag values, 8 bits are available.
*/
--
2.26.2
--------------19F6AF79E033375862CBEBB4
Content-Type: text/x-patch; charset=UTF-8;
name="0002_handle_HEAP_INSERT_FROZEN_in_heap_insert.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0002_handle_HEAP_INSERT_FROZEN_in_heap_insert.patch"
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 32cc010cb7..3663ff4b83 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1862,8 +1862,12 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
TransactionId xid = GetCurrentTransactionId();
HeapTuple heaptup;
Buffer buffer;
+ Page page;
Buffer vmbuffer = InvalidBuffer;
+ bool starting_with_empty_page;
bool all_visible_cleared = false;
+ bool all_frozen_set = false;
+ uint8 vmstatus = 0;
/*
* Fill in tuple header fields and toast the tuple if necessary.
@@ -1876,11 +1880,36 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
/*
* Find buffer to insert this tuple into. If the page is all visible,
* this will also pin the requisite visibility map page.
+ *
+ * Also pin visibility map page if COPY FREEZE inserts tuples into an
+ * empty page. See all_frozen_set below.
*/
buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
InvalidBuffer, options, bistate,
&vmbuffer, NULL);
+
+ /*
+ * If we're inserting frozen entry into an empty page,
+ * set visibility map bits and PageAllVisible() hint.
+ *
+ * If we're inserting frozen entry into already all_frozen page,
+ * preserve this state.
+ */
+ if (options & HEAP_INSERT_FROZEN)
+ {
+ page = BufferGetPage(buffer);
+
+ starting_with_empty_page = PageGetMaxOffsetNumber(page) == 0;
+
+ if (visibilitymap_pin_ok(BufferGetBlockNumber(buffer), vmbuffer))
+ vmstatus = visibilitymap_get_status(relation,
+ BufferGetBlockNumber(buffer), &vmbuffer);
+
+ if ((starting_with_empty_page || vmstatus & VISIBILITYMAP_ALL_FROZEN))
+ all_frozen_set = true;
+ }
+
/*
* We're about to do the actual insert -- but check for conflict first, to
* avoid possibly having to roll back work we've just done.
@@ -1904,7 +1933,14 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
RelationPutHeapTuple(relation, buffer, heaptup,
(options & HEAP_INSERT_SPECULATIVE) != 0);
- if (PageIsAllVisible(BufferGetPage(buffer)))
+ /*
+ * If the page is all visible, need to clear that, unless we're only
+ * going to add further frozen rows to it.
+ *
+ * If we're only adding already frozen rows to a previously empty
+ * or all visible page, mark it as all-visible.
+ */
+ if (PageIsAllVisible(BufferGetPage(buffer)) && !(options & HEAP_INSERT_FROZEN))
{
all_visible_cleared = true;
PageClearAllVisible(BufferGetPage(buffer));
@@ -1912,6 +1948,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
ItemPointerGetBlockNumber(&(heaptup->t_self)),
vmbuffer, VISIBILITYMAP_VALID_BITS);
}
+ else if (all_frozen_set)
+ PageSetAllVisible(page);
/*
* XXX Should we set PageSetPrunable on this page ?
@@ -1959,6 +1997,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
xlrec.flags = 0;
if (all_visible_cleared)
xlrec.flags |= XLH_INSERT_ALL_VISIBLE_CLEARED;
+ if (all_frozen_set)
+ xlrec.flags = XLH_INSERT_ALL_FROZEN_SET;
if (options & HEAP_INSERT_SPECULATIVE)
xlrec.flags |= XLH_INSERT_IS_SPECULATIVE;
Assert(ItemPointerGetBlockNumber(&heaptup->t_self) == BufferGetBlockNumber(buffer));
@@ -2007,6 +2047,29 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
END_CRIT_SECTION();
+ /*
+ * If we've frozen everything on the page, update the visibilitymap.
+ * We're already holding pin on the vmbuffer.
+ *
+ * No need to update the visibilitymap if it had all_frozen bit set
+ * before this insertion.
+ */
+ if (all_frozen_set && ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0))
+ {
+ Assert(PageIsAllVisible(page));
+ Assert(visibilitymap_pin_ok(BufferGetBlockNumber(buffer), vmbuffer));
+
+ /*
+ * It's fine to use InvalidTransactionId here - this is only used
+ * when HEAP_INSERT_FROZEN is specified, which intentionally
+ * violates visibility rules.
+ */
+ visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
+ InvalidXLogRecPtr, vmbuffer,
+ InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
+ }
+
UnlockReleaseBuffer(buffer);
if (vmbuffer != InvalidBuffer)
ReleaseBuffer(vmbuffer);
@@ -8197,6 +8260,10 @@ heap_xlog_insert(XLogReaderState *record)
ItemPointerSetBlockNumber(&target_tid, blkno);
ItemPointerSetOffsetNumber(&target_tid, xlrec->offnum);
+ /* check that the mutually exclusive flags are not both set */
+ Assert (!((xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) &&
+ (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET)));
+
/*
* The visibility map may need to be fixed even if the heap page is
* already up-to-date.
@@ -8267,6 +8334,11 @@ heap_xlog_insert(XLogReaderState *record)
if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
PageClearAllVisible(page);
+ /* XLH_INSERT_ALL_FROZEN_SET implies that all tuples are visible */
+ if (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET)
+ PageSetAllVisible(page);
+
+
MarkBufferDirty(buffer);
}
if (BufferIsValid(buffer))
--------------19F6AF79E033375862CBEBB4--
^ permalink raw reply [nested|flat] 6+ messages in thread
* Strange assertion in procarray.c
@ 2024-11-25 19:38 Michail Nikolaev <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michail Nikolaev @ 2024-11-25 19:38 UTC (permalink / raw)
To: pgsql-hackers
Hello, everyone!
While working on stabilization of tests for [0] I noticed a strange assert
happens in procarray.c [1].
It looks like this:
[562801][client backend] [isolation/two-ids/s2][50/131:536136]
ERROR: could not serialize access due to read/write dependencies among
transactions
[562801][client backend] [isolation/two-ids/s2][50/131:536136]
DETAIL: Reason code: Canceled on identification as a pivot, during commit
attempt.
[562801][client backend] [isolation/two-ids/s2][50/131:536136]
HINT: The transaction might succeed if retried.
[562801][client backend] [isolation/two-ids/s2][50/131:536136]
STATEMENT: COMMIT;
[562801][client backend] [isolation/two-ids/s2][50/0:536136] ERROR:
ResourceOwnerEnlarge called after release started
[562801][client backend] [isolation/two-ids/s2][50/0:536136]
WARNING: AbortTransaction while in ABORT state
TRAP: failed Assert("TransactionIdIsValid(proc->xid)"), File:
"../src/backend/storage/ipc/procarray.c", Line: 677, PID: 562801
[562819][client backend] [pg_regress/test_parser][:0] LOG:
disconnection: session time: 0:00:00.011 user=someone
database=regression_test_parser host=[local]
postgres: someone isolation_regression [local]
COMMIT(ExceptionalCondition+0xbe)[0x55f2a101f185]
postgres: someone isolation_regression [local]
COMMIT(ProcArrayEndTransaction+0x46)[0x55f2a0ddf7b3]
postgres: someone isolation_regression [local]
COMMIT(+0x1e29b1)[0x55f2a09e59b1]
postgres: someone isolation_regression [local]
COMMIT(+0x1e347b)[0x55f2a09e647b]
postgres: someone isolation_regression [local]
COMMIT(AbortCurrentTransaction+0xe)[0x55f2a09e63a3]
postgres: someone isolation_regression [local]
COMMIT(PostgresMain+0x538)[0x55f2a0e20ff1]
postgres: someone isolation_regression [local]
COMMIT(+0x61457b)[0x55f2a0e1757b]
postgres: someone isolation_regression [local]
COMMIT(postmaster_child_launch+0x137)[0x55f2a0d295bf]
postgres: someone isolation_regression [local]
COMMIT(+0x52cff5)[0x55f2a0d2fff5]
postgres: someone isolation_regression [local]
COMMIT(+0x52a6cd)[0x55f2a0d2d6cd]
postgres: someone isolation_regression [local]
COMMIT(PostmasterMain+0x1629)[0x55f2a0d2cfae]
postgres: someone isolation_regression [local]
COMMIT(+0x404ba2)[0x55f2a0c07ba2]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7f6afbe7e1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7f6afbe7e28b]
postgres: someone isolation_regression [local]
COMMIT(_start+0x25)[0x55f2a08e3ab5]
I made a reproducer for that. Ignore index_concurrently_upsert - it should
fail. Also, some source files are changed - but it is only injection points.
But in several cases of "meson test --print-errorlogs --num-processes=8
--setup running" build backend crashes. I was unable to reproduce it during
"non-running" tests.
A full backend log for the crash run is attached.
There are some helpful commands to reproduce locally:
cd build
ninja && meson test --suite setup
cd ../
export
LD_LIBRARY_PATH="$(pwd)/build/tmp_install/usr/local/pgsql/lib/x86_64-linux-gnu:$LD_LIBRARY_PATH"
build/tmp_install/usr/local/pgsql/bin/initdb -N build/runningcheck
--no-instructions -A trust
echo "include '$(pwd)/src/tools/ci/pg_ci_base.conf'" >>
build/runningcheck/postgresql.conf
build/tmp_install/usr/local/pgsql/bin/pg_ctl -c -o '-c fsync=off'
-D build/runningcheck -l build/testrun/runningcheck.log start
cd build
meson test --print-errorlogs --num-processes=8 --setup running
Best regards,
Mikhail.
[0]: https://commitfest.postgresql.org/50/5160/
[1]:
https://github.com/postgres/postgres/blob/478846e7688c9ab73d2695a66822e9ae0574b551/src/backend/stora...
From 7ed9e2409d6b0fc4e4a915ba45232a10326c778b Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Mon, 25 Nov 2024 19:13:40 +0100
Subject: [PATCH v1] meson test --print-errorlogs --num-processes=8 --setup
running
reproducer for strange assert:
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] ERROR: could not serialize access due to read/write dependencies among transactions
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] HINT: The transaction might succeed if retried.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] STATEMENT: COMMIT;
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] ERROR: ResourceOwnerEnlarge called after release started
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] WARNING: AbortTransaction while in ABORT state
TRAP: failed Assert("TransactionIdIsValid(proc->xid)"), File: "../src/backend/storage/ipc/procarray.c", Line: 677, PID: 562801
2024-11-25 19:08:12.064 CET [562819][client backend] [pg_regress/test_parser][:0] LOG: disconnection: session time: 0:00:00.011 user=nkey database=regression_test_parser host=[local]
postgres: nkey isolation_regression [local] COMMIT(ExceptionalCondition+0xbe)[0x55f2a101f185]
postgres: nkey isolation_regression [local] COMMIT(ProcArrayEndTransaction+0x46)[0x55f2a0ddf7b3]
postgres: nkey isolation_regression [local] COMMIT(+0x1e29b1)[0x55f2a09e59b1]
postgres: nkey isolation_regression [local] COMMIT(+0x1e347b)[0x55f2a09e647b]
postgres: nkey isolation_regression [local] COMMIT(AbortCurrentTransaction+0xe)[0x55f2a09e63a3]
postgres: nkey isolation_regression [local] COMMIT(PostgresMain+0x538)[0x55f2a0e20ff1]
postgres: nkey isolation_regression [local] COMMIT(+0x61457b)[0x55f2a0e1757b]
postgres: nkey isolation_regression [local] COMMIT(postmaster_child_launch+0x137)[0x55f2a0d295bf]
postgres: nkey isolation_regression [local] COMMIT(+0x52cff5)[0x55f2a0d2fff5]
postgres: nkey isolation_regression [local] COMMIT(+0x52a6cd)[0x55f2a0d2d6cd]
postgres: nkey isolation_regression [local] COMMIT(PostmasterMain+0x1629)[0x55f2a0d2cfae]
postgres: nkey isolation_regression [local] COMMIT(+0x404ba2)[0x55f2a0c07ba2]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7f6afbe7e1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7f6afbe7e28b]
postgres: nkey isolation_regression [local] COMMIT(_start+0x25)[0x55f2a08e3ab5]
---
src/backend/commands/indexcmds.c | 4 +-
src/backend/executor/execIndexing.c | 3 +
src/backend/executor/nodeModifyTable.c | 2 +
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../expected/index_concurrently_upsert.out | 80 +++++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/index_concurrently_upsert.spec | 68 ++++++++++++++++
8 files changed, 160 insertions(+), 2 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/index_concurrently_upsert.out
create mode 100644 src/test/modules/injection_points/specs/index_concurrently_upsert.spec
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d1134733c17..8f48f14eddd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1766,6 +1766,7 @@ DefineIndex(Oid tableId,
* before the reference snap was taken, we have to wait out any
* transactions that might have older snapshots.
*/
+ INJECTION_POINT("define_index_before_set_valid");
pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_PHASE_WAIT_3);
WaitForOlderSnapshots(limitXmin, true);
@@ -4206,7 +4207,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* the same time to make sure we only get constraint violations from the
* indexes with the correct names.
*/
-
+ INJECTION_POINT("reindex_relation_concurrently_before_swap");
StartTransactionCommand();
/*
@@ -4285,6 +4286,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* index_drop() for more details.
*/
+ INJECTION_POINT("reindex_relation_concurrently_before_set_dead");
pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_PHASE_WAIT_4);
WaitForLockersMultiple(lockTags, AccessExclusiveLock, true);
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index f9a2fac79e4..5d04f189340 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -117,6 +117,7 @@
#include "utils/multirangetypes.h"
#include "utils/rangetypes.h"
#include "utils/snapmgr.h"
+#include "utils/injection_point.h"
/* waitMode argument to check_exclusion_or_unique_constraint() */
typedef enum
@@ -936,6 +937,8 @@ retry:
econtext->ecxt_scantuple = save_scantuple;
ExecDropSingleTupleTableSlot(existing_slot);
+ if (!conflict)
+ INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict");
return !conflict;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1161520f76b..23cf4c6b540 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -69,6 +69,7 @@
#include "utils/datum.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
+#include "utils/injection_point.h"
typedef struct MTTargetRelLookup
@@ -1087,6 +1088,7 @@ ExecInsert(ModifyTableContext *context,
return NULL;
}
}
+ INJECTION_POINT("exec_insert_before_insert_speculative");
/*
* Before we start insertion proper, acquire our "speculative
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..3bde3e67503 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic inplace index_concurrently_upsert
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/index_concurrently_upsert.out b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
new file mode 100644
index 00000000000..42dd059d985
--- /dev/null
+++ b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
@@ -0,0 +1,80 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s3_start_create_index s1_start_upsert s4_wakeup_define_index_before_set_valid s2_start_upsert s4_wakeup_s1_from_invalidate_catalog_snapshot s4_wakeup_s2 s4_wakeup_s1
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s3_start_create_index: CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); <waiting ...>
+step s1_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_define_index_before_set_valid:
+ SELECT injection_points_detach('define_index_before_set_valid');
+ SELECT injection_points_wakeup('define_index_before_set_valid');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s2_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_s1_from_invalidate_catalog_snapshot:
+ SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+ SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_wakeup_s2:
+ SELECT injection_points_detach('exec_insert_before_insert_speculative');
+ SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s2_start_upsert: <... completed>
+step s4_wakeup_s1:
+ SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+ SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s3_start_create_index: <... completed>
+step s1_start_upsert: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..b50140bcf4a 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -44,6 +44,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'index_concurrently_upsert',
],
},
'tap': {
diff --git a/src/test/modules/injection_points/specs/index_concurrently_upsert.spec b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
new file mode 100644
index 00000000000..075450935b6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
@@ -0,0 +1,68 @@
+# Test race conditions involving:
+# - s1: UPSERT a tuple
+# - s2: UPSERT the same tuple
+# - s3: CREATE UNIQUE INDEX CONCURRENTLY
+# - s4: operations with injection points
+
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE SCHEMA test;
+ CREATE UNLOGGED TABLE test.tbl(i int primary key, updated_at timestamp);
+ ALTER TABLE test.tbl SET (parallel_workers=0);
+}
+
+teardown
+{
+ DROP SCHEMA test CASCADE;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'wait');
+ SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+}
+step s1_start_upsert { INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s2
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('exec_insert_before_insert_speculative', 'wait');
+}
+step s2_start_upsert { INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s3
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('define_index_before_set_valid', 'wait');
+}
+step s3_start_create_index { CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); }
+
+session s4
+step s4_wakeup_s1 {
+ SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+ SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+}
+step s4_wakeup_s1_from_invalidate_catalog_snapshot {
+ SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+ SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+}
+step s4_wakeup_s2 {
+ SELECT injection_points_detach('exec_insert_before_insert_speculative');
+ SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+}
+step s4_wakeup_define_index_before_set_valid {
+ SELECT injection_points_detach('define_index_before_set_valid');
+ SELECT injection_points_wakeup('define_index_before_set_valid');
+}
+
+permutation
+ s3_start_create_index
+ s1_start_upsert
+ s4_wakeup_define_index_before_set_valid
+ s2_start_upsert
+ s4_wakeup_s1_from_invalidate_catalog_snapshot
+ s4_wakeup_s2
+ s4_wakeup_s1
\ No newline at end of file
--
2.43.0
Attachments:
[text/plain] v1-0001-meson-test-print-errorlogs-num-processes-8-setup-.patch (12.5K, ../../CANtu0oiTgFW47QgpTwrMOVm3Bq4N0Y5bjvTy5sP0gYWLQuVgjw@mail.gmail.com/3-v1-0001-meson-test-print-errorlogs-num-processes-8-setup-.patch)
download | inline diff:
From 7ed9e2409d6b0fc4e4a915ba45232a10326c778b Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Mon, 25 Nov 2024 19:13:40 +0100
Subject: [PATCH v1] meson test --print-errorlogs --num-processes=8 --setup
running
reproducer for strange assert:
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] ERROR: could not serialize access due to read/write dependencies among transactions
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] DETAIL: Reason code: Canceled on identification as a pivot, during commit attempt.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] HINT: The transaction might succeed if retried.
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/131:536136] STATEMENT: COMMIT;
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] ERROR: ResourceOwnerEnlarge called after release started
2024-11-25 19:08:12.063 CET [562801][client backend] [isolation/two-ids/s2][50/0:536136] WARNING: AbortTransaction while in ABORT state
TRAP: failed Assert("TransactionIdIsValid(proc->xid)"), File: "../src/backend/storage/ipc/procarray.c", Line: 677, PID: 562801
2024-11-25 19:08:12.064 CET [562819][client backend] [pg_regress/test_parser][:0] LOG: disconnection: session time: 0:00:00.011 user=nkey database=regression_test_parser host=[local]
postgres: nkey isolation_regression [local] COMMIT(ExceptionalCondition+0xbe)[0x55f2a101f185]
postgres: nkey isolation_regression [local] COMMIT(ProcArrayEndTransaction+0x46)[0x55f2a0ddf7b3]
postgres: nkey isolation_regression [local] COMMIT(+0x1e29b1)[0x55f2a09e59b1]
postgres: nkey isolation_regression [local] COMMIT(+0x1e347b)[0x55f2a09e647b]
postgres: nkey isolation_regression [local] COMMIT(AbortCurrentTransaction+0xe)[0x55f2a09e63a3]
postgres: nkey isolation_regression [local] COMMIT(PostgresMain+0x538)[0x55f2a0e20ff1]
postgres: nkey isolation_regression [local] COMMIT(+0x61457b)[0x55f2a0e1757b]
postgres: nkey isolation_regression [local] COMMIT(postmaster_child_launch+0x137)[0x55f2a0d295bf]
postgres: nkey isolation_regression [local] COMMIT(+0x52cff5)[0x55f2a0d2fff5]
postgres: nkey isolation_regression [local] COMMIT(+0x52a6cd)[0x55f2a0d2d6cd]
postgres: nkey isolation_regression [local] COMMIT(PostmasterMain+0x1629)[0x55f2a0d2cfae]
postgres: nkey isolation_regression [local] COMMIT(+0x404ba2)[0x55f2a0c07ba2]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7f6afbe7e1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7f6afbe7e28b]
postgres: nkey isolation_regression [local] COMMIT(_start+0x25)[0x55f2a08e3ab5]
---
src/backend/commands/indexcmds.c | 4 +-
src/backend/executor/execIndexing.c | 3 +
src/backend/executor/nodeModifyTable.c | 2 +
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../expected/index_concurrently_upsert.out | 80 +++++++++++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/index_concurrently_upsert.spec | 68 ++++++++++++++++
8 files changed, 160 insertions(+), 2 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/index_concurrently_upsert.out
create mode 100644 src/test/modules/injection_points/specs/index_concurrently_upsert.spec
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d1134733c17..8f48f14eddd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1766,6 +1766,7 @@ DefineIndex(Oid tableId,
* before the reference snap was taken, we have to wait out any
* transactions that might have older snapshots.
*/
+ INJECTION_POINT("define_index_before_set_valid");
pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_PHASE_WAIT_3);
WaitForOlderSnapshots(limitXmin, true);
@@ -4206,7 +4207,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* the same time to make sure we only get constraint violations from the
* indexes with the correct names.
*/
-
+ INJECTION_POINT("reindex_relation_concurrently_before_swap");
StartTransactionCommand();
/*
@@ -4285,6 +4286,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* index_drop() for more details.
*/
+ INJECTION_POINT("reindex_relation_concurrently_before_set_dead");
pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_PHASE_WAIT_4);
WaitForLockersMultiple(lockTags, AccessExclusiveLock, true);
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index f9a2fac79e4..5d04f189340 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -117,6 +117,7 @@
#include "utils/multirangetypes.h"
#include "utils/rangetypes.h"
#include "utils/snapmgr.h"
+#include "utils/injection_point.h"
/* waitMode argument to check_exclusion_or_unique_constraint() */
typedef enum
@@ -936,6 +937,8 @@ retry:
econtext->ecxt_scantuple = save_scantuple;
ExecDropSingleTupleTableSlot(existing_slot);
+ if (!conflict)
+ INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict");
return !conflict;
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 1161520f76b..23cf4c6b540 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -69,6 +69,7 @@
#include "utils/datum.h"
#include "utils/rel.h"
#include "utils/snapmgr.h"
+#include "utils/injection_point.h"
typedef struct MTTargetRelLookup
@@ -1087,6 +1088,7 @@ ExecInsert(ModifyTableContext *context,
return NULL;
}
}
+ INJECTION_POINT("exec_insert_before_insert_speculative");
/*
* Before we start insertion proper, acquire our "speculative
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..3bde3e67503 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic inplace index_concurrently_upsert
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/index_concurrently_upsert.out b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
new file mode 100644
index 00000000000..42dd059d985
--- /dev/null
+++ b/src/test/modules/injection_points/expected/index_concurrently_upsert.out
@@ -0,0 +1,80 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s3_start_create_index s1_start_upsert s4_wakeup_define_index_before_set_valid s2_start_upsert s4_wakeup_s1_from_invalidate_catalog_snapshot s4_wakeup_s2 s4_wakeup_s1
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s3_start_create_index: CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); <waiting ...>
+step s1_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_define_index_before_set_valid:
+ SELECT injection_points_detach('define_index_before_set_valid');
+ SELECT injection_points_wakeup('define_index_before_set_valid');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s2_start_upsert: INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); <waiting ...>
+step s4_wakeup_s1_from_invalidate_catalog_snapshot:
+ SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+ SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_wakeup_s2:
+ SELECT injection_points_detach('exec_insert_before_insert_speculative');
+ SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s2_start_upsert: <... completed>
+step s4_wakeup_s1:
+ SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+ SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s3_start_create_index: <... completed>
+step s1_start_upsert: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..b50140bcf4a 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -44,6 +44,7 @@ tests += {
'specs': [
'basic',
'inplace',
+ 'index_concurrently_upsert',
],
},
'tap': {
diff --git a/src/test/modules/injection_points/specs/index_concurrently_upsert.spec b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
new file mode 100644
index 00000000000..075450935b6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/index_concurrently_upsert.spec
@@ -0,0 +1,68 @@
+# Test race conditions involving:
+# - s1: UPSERT a tuple
+# - s2: UPSERT the same tuple
+# - s3: CREATE UNIQUE INDEX CONCURRENTLY
+# - s4: operations with injection points
+
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE SCHEMA test;
+ CREATE UNLOGGED TABLE test.tbl(i int primary key, updated_at timestamp);
+ ALTER TABLE test.tbl SET (parallel_workers=0);
+}
+
+teardown
+{
+ DROP SCHEMA test CASCADE;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'wait');
+ SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+}
+step s1_start_upsert { INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s2
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('exec_insert_before_insert_speculative', 'wait');
+}
+step s2_start_upsert { INSERT INTO test.tbl VALUES(13,now()) on conflict(i) do update set updated_at = now(); }
+
+session s3
+setup {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('define_index_before_set_valid', 'wait');
+}
+step s3_start_create_index { CREATE UNIQUE INDEX CONCURRENTLY tbl_pkey_duplicate ON test.tbl(i); }
+
+session s4
+step s4_wakeup_s1 {
+ SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict');
+ SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_no_conflict');
+}
+step s4_wakeup_s1_from_invalidate_catalog_snapshot {
+ SELECT injection_points_detach('invalidate_catalog_snapshot_end');
+ SELECT injection_points_wakeup('invalidate_catalog_snapshot_end');
+}
+step s4_wakeup_s2 {
+ SELECT injection_points_detach('exec_insert_before_insert_speculative');
+ SELECT injection_points_wakeup('exec_insert_before_insert_speculative');
+}
+step s4_wakeup_define_index_before_set_valid {
+ SELECT injection_points_detach('define_index_before_set_valid');
+ SELECT injection_points_wakeup('define_index_before_set_valid');
+}
+
+permutation
+ s3_start_create_index
+ s1_start_upsert
+ s4_wakeup_define_index_before_set_valid
+ s2_start_upsert
+ s4_wakeup_s1_from_invalidate_catalog_snapshot
+ s4_wakeup_s2
+ s4_wakeup_s1
\ No newline at end of file
--
2.43.0
[application/octet-stream] runningcheck.log (1.7M, ../../CANtu0oiTgFW47QgpTwrMOVm3Bq4N0Y5bjvTy5sP0gYWLQuVgjw@mail.gmail.com/4-runningcheck.log)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Strange assertion in procarray.c
@ 2024-11-27 02:27 Michail Nikolaev <[email protected]>
parent: Michail Nikolaev <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michail Nikolaev @ 2024-11-27 02:27 UTC (permalink / raw)
To: pgsql-hackers; [email protected]; Michael Paquier <[email protected]>
Hello, Nathan and Michael!
I believe I’ve identified the root cause of the issue. It appears to be
related to the GetNamedDSMSegment and injection_points module.
Here’s how it happens:
* A backend attaches (even locally!) to an injection point, which might get
triggered during resource release (ResourceOwnerRelease).
* Another backend attempts to release the same resource (e.g., by aborting
a transaction) and triggers the injection point.
* This leads to a call to GetNamedDSMSegment, as it’s the first time this
backend interacts with injection points.
* Consequently, an assertion failure occurs in ResourceOwnerEnlarge because
the backend is in the process of releasing all resources.
I’ve attached a reproducer—this version is much simpler for debugging.
So, it looks like we need to provide some option to guarantee
GetNamedDSMSegment called for injection points module.
Any other ideas?
Best regards,
Mikhail.
[0]:
https://github.com/postgres/postgres/commit/8b2bcf3f287c79eaebf724cba57e5ff664b01e06
From e56b6de29aea01916d35d22e9a59241a04202b37 Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Wed, 27 Nov 2024 02:10:17 +0100
Subject: [PATCH v2] Test to reproduce issue with crash caused by passing throw
injection points during transaction aborting (caused by call to
injection_init_shmem).
---
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/crash.out | 26 ++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/specs/crash.spec | 47 +++++++++++++++++++
5 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/crash.out
create mode 100644 src/test/modules/injection_points/specs/crash.spec
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..da8930ea49f 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic crash inplace
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/crash.out b/src/test/modules/injection_points/expected/crash.out
new file mode 100644
index 00000000000..7d7f298c786
--- /dev/null
+++ b/src/test/modules/injection_points/expected/crash.out
@@ -0,0 +1,26 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s4_attach_locally wx1 rxwy2 c1 ry3 c2 c3
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+step s4_attach_locally: SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wx1: update D1 set id = id + 1;
+step rxwy2: update D2 set id = (select id+1 from D1);
+step c1: COMMIT;
+step ry3: select id from D2;
+id
+--
+ 1
+(1 row)
+
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step c3: COMMIT;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..6079187dd57 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'crash',
'inplace',
],
},
diff --git a/src/test/modules/injection_points/specs/crash.spec b/src/test/modules/injection_points/specs/crash.spec
new file mode 100644
index 00000000000..55e6a5434ab
--- /dev/null
+++ b/src/test/modules/injection_points/specs/crash.spec
@@ -0,0 +1,47 @@
+setup
+{
+ create table D1 (id int primary key not null);
+ create table D2 (id int primary key not null);
+ insert into D1 values (1);
+ insert into D2 values (1);
+
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP TABLE D1, D2;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE;}
+step wx1 { update D1 set id = id + 1; }
+step c1 { COMMIT; }
+
+session s2
+setup {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+}
+step rxwy2 { update D2 set id = (select id+1 from D1); }
+step c2 { COMMIT; }
+
+session s3
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step ry3 { select id from D2; }
+step c3 { COMMIT; }
+
+session s4
+setup {
+ SELECT injection_points_set_local();
+}
+step s4_attach_locally { SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
+
+permutation
+ s4_attach_locally
+ wx1
+ rxwy2
+ c1
+ ry3
+ c2
+ c3
\ No newline at end of file
--
2.43.0
Attachments:
[text/plain] v2-0001-Test-to-reproduce-issue-with-crash-caused-by-pass.patch (4.3K, ../../CANtu0ojbx6=esP8euQgzD1CN6tigTQvDmupwEmLTHZT=6_yx_A@mail.gmail.com/3-v2-0001-Test-to-reproduce-issue-with-crash-caused-by-pass.patch)
download | inline diff:
From e56b6de29aea01916d35d22e9a59241a04202b37 Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Wed, 27 Nov 2024 02:10:17 +0100
Subject: [PATCH v2] Test to reproduce issue with crash caused by passing throw
injection points during transaction aborting (caused by call to
injection_init_shmem).
---
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/crash.out | 26 ++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/specs/crash.spec | 47 +++++++++++++++++++
5 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/crash.out
create mode 100644 src/test/modules/injection_points/specs/crash.spec
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..da8930ea49f 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic crash inplace
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/crash.out b/src/test/modules/injection_points/expected/crash.out
new file mode 100644
index 00000000000..7d7f298c786
--- /dev/null
+++ b/src/test/modules/injection_points/expected/crash.out
@@ -0,0 +1,26 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s4_attach_locally wx1 rxwy2 c1 ry3 c2 c3
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+step s4_attach_locally: SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wx1: update D1 set id = id + 1;
+step rxwy2: update D2 set id = (select id+1 from D1);
+step c1: COMMIT;
+step ry3: select id from D2;
+id
+--
+ 1
+(1 row)
+
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step c3: COMMIT;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..6079187dd57 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'crash',
'inplace',
],
},
diff --git a/src/test/modules/injection_points/specs/crash.spec b/src/test/modules/injection_points/specs/crash.spec
new file mode 100644
index 00000000000..55e6a5434ab
--- /dev/null
+++ b/src/test/modules/injection_points/specs/crash.spec
@@ -0,0 +1,47 @@
+setup
+{
+ create table D1 (id int primary key not null);
+ create table D2 (id int primary key not null);
+ insert into D1 values (1);
+ insert into D2 values (1);
+
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP TABLE D1, D2;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE;}
+step wx1 { update D1 set id = id + 1; }
+step c1 { COMMIT; }
+
+session s2
+setup {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+}
+step rxwy2 { update D2 set id = (select id+1 from D1); }
+step c2 { COMMIT; }
+
+session s3
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step ry3 { select id from D2; }
+step c3 { COMMIT; }
+
+session s4
+setup {
+ SELECT injection_points_set_local();
+}
+step s4_attach_locally { SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
+
+permutation
+ s4_attach_locally
+ wx1
+ rxwy2
+ c1
+ ry3
+ c2
+ c3
\ No newline at end of file
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Strange assertion in procarray.c
@ 2024-11-27 09:50 Michail Nikolaev <[email protected]>
parent: Michail Nikolaev <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michail Nikolaev @ 2024-11-27 09:50 UTC (permalink / raw)
To: pgsql-hackers; [email protected]; Michael Paquier <[email protected]>
Hello, again.
> Another backend attempts to release the same resource (e.g., by aborting
a transaction) and triggers the injection point.
Oh, all that GPT-like correctors required to be carefully checked :)
Correct version: Another backend attempts to release some resource (e.g.,
by aborting a transaction) and triggers the injection point.
Best regards,
Mikhail.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Strange assertion in procarray.c
@ 2024-11-28 20:04 Michail Nikolaev <[email protected]>
parent: Michail Nikolaev <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Michail Nikolaev @ 2024-11-28 20:04 UTC (permalink / raw)
To: pgsql-hackers; [email protected]; Michael Paquier <[email protected]>; Heikki Linnakangas <[email protected]>
Hello, Heikki, Nathan and Michael!
Oh, please excuse my impudence in bringing you all here, but I finally
found what almost the same issue was fixed by Heikki already [0].
I discovered that a similar issue was previously addressed by Heikki in
commit [0], where installcheck was disabled for injection point tests.
However, in the meson build configuration, this was only applied to
regression tests - the isolation and TAP tests are still running during
installcheck.
As demonstrated in the previously shared reproducer [1], even *local*
injection points can cause backend crashes through unexpected side effects.
Therefore, I propose extending the installcheck disable to cover both TAP
and isolation tests as well.
I've attached a patch implementing these changes.
A patch with such change is attached.
Best regards,
Mikhail.
[0]:
https://github.com/postgres/postgres/commit/e2e3b8ae9ed73fcd3096c5ca93971891a7767388
[1]:
https://www.postgresql.org/message-id/flat/CANtu0ojbx6%3DesP8euQgzD1CN6tigTQvDmupwEmLTHZT%3D6_yx_A%4...
>
From e56b6de29aea01916d35d22e9a59241a04202b37 Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Wed, 27 Nov 2024 02:10:17 +0100
Subject: [PATCH v2] Test to reproduce issue with crash caused by passing throw
injection points during transaction aborting (caused by call to
injection_init_shmem).
---
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/crash.out | 26 ++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/specs/crash.spec | 47 +++++++++++++++++++
5 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/crash.out
create mode 100644 src/test/modules/injection_points/specs/crash.spec
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..da8930ea49f 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic crash inplace
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/crash.out b/src/test/modules/injection_points/expected/crash.out
new file mode 100644
index 00000000000..7d7f298c786
--- /dev/null
+++ b/src/test/modules/injection_points/expected/crash.out
@@ -0,0 +1,26 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s4_attach_locally wx1 rxwy2 c1 ry3 c2 c3
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+step s4_attach_locally: SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wx1: update D1 set id = id + 1;
+step rxwy2: update D2 set id = (select id+1 from D1);
+step c1: COMMIT;
+step ry3: select id from D2;
+id
+--
+ 1
+(1 row)
+
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step c3: COMMIT;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..6079187dd57 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'crash',
'inplace',
],
},
diff --git a/src/test/modules/injection_points/specs/crash.spec b/src/test/modules/injection_points/specs/crash.spec
new file mode 100644
index 00000000000..55e6a5434ab
--- /dev/null
+++ b/src/test/modules/injection_points/specs/crash.spec
@@ -0,0 +1,47 @@
+setup
+{
+ create table D1 (id int primary key not null);
+ create table D2 (id int primary key not null);
+ insert into D1 values (1);
+ insert into D2 values (1);
+
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP TABLE D1, D2;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE;}
+step wx1 { update D1 set id = id + 1; }
+step c1 { COMMIT; }
+
+session s2
+setup {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+}
+step rxwy2 { update D2 set id = (select id+1 from D1); }
+step c2 { COMMIT; }
+
+session s3
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step ry3 { select id from D2; }
+step c3 { COMMIT; }
+
+session s4
+setup {
+ SELECT injection_points_set_local();
+}
+step s4_attach_locally { SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
+
+permutation
+ s4_attach_locally
+ wx1
+ rxwy2
+ c1
+ ry3
+ c2
+ c3
\ No newline at end of file
--
2.43.0
Attachments:
[text/plain] v2-0001-Test-to-reproduce-issue-with-crash-caused-by-pass.patch (4.3K, ../../CANtu0oiBAcVnzYYETbWY+2gFXUeAx8BKArjnFco4LeAHfH38Sw@mail.gmail.com/3-v2-0001-Test-to-reproduce-issue-with-crash-caused-by-pass.patch)
download | inline diff:
From e56b6de29aea01916d35d22e9a59241a04202b37 Mon Sep 17 00:00:00 2001
From: nkey <[email protected]>
Date: Wed, 27 Nov 2024 02:10:17 +0100
Subject: [PATCH v2] Test to reproduce issue with crash caused by passing throw
injection points during transaction aborting (caused by call to
injection_init_shmem).
---
src/backend/utils/time/snapmgr.c | 2 +
src/test/modules/injection_points/Makefile | 2 +-
.../injection_points/expected/crash.out | 26 ++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../modules/injection_points/specs/crash.spec | 47 +++++++++++++++++++
5 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/crash.out
create mode 100644 src/test/modules/injection_points/specs/crash.spec
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 7d2b34d4f20..3a7357a050d 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -64,6 +64,7 @@
#include "utils/resowner.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
+#include "utils/injection_point.h"
/*
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
}
}
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 0753a9df58c..da8930ea49f 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,7 +13,7 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points reindex_conc
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
-ISOLATION = basic inplace
+ISOLATION = basic crash inplace
TAP_TESTS = 1
diff --git a/src/test/modules/injection_points/expected/crash.out b/src/test/modules/injection_points/expected/crash.out
new file mode 100644
index 00000000000..7d7f298c786
--- /dev/null
+++ b/src/test/modules/injection_points/expected/crash.out
@@ -0,0 +1,26 @@
+Parsed test spec with 4 sessions
+
+starting permutation: s4_attach_locally wx1 rxwy2 c1 ry3 c2 c3
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+step s4_attach_locally: SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait');
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step wx1: update D1 set id = id + 1;
+step rxwy2: update D2 set id = (select id+1 from D1);
+step c1: COMMIT;
+step ry3: select id from D2;
+id
+--
+ 1
+(1 row)
+
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step c3: COMMIT;
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 58f19001157..6079187dd57 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -43,6 +43,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'crash',
'inplace',
],
},
diff --git a/src/test/modules/injection_points/specs/crash.spec b/src/test/modules/injection_points/specs/crash.spec
new file mode 100644
index 00000000000..55e6a5434ab
--- /dev/null
+++ b/src/test/modules/injection_points/specs/crash.spec
@@ -0,0 +1,47 @@
+setup
+{
+ create table D1 (id int primary key not null);
+ create table D2 (id int primary key not null);
+ insert into D1 values (1);
+ insert into D2 values (1);
+
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP TABLE D1, D2;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE;}
+step wx1 { update D1 set id = id + 1; }
+step c1 { COMMIT; }
+
+session s2
+setup {
+ BEGIN ISOLATION LEVEL SERIALIZABLE;
+}
+step rxwy2 { update D2 set id = (select id+1 from D1); }
+step c2 { COMMIT; }
+
+session s3
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step ry3 { select id from D2; }
+step c3 { COMMIT; }
+
+session s4
+setup {
+ SELECT injection_points_set_local();
+}
+step s4_attach_locally { SELECT injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
+
+permutation
+ s4_attach_locally
+ wx1
+ rxwy2
+ c1
+ ry3
+ c2
+ c3
\ No newline at end of file
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Strange assertion in procarray.c
@ 2024-11-29 05:06 Michael Paquier <[email protected]>
parent: Michail Nikolaev <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Michael Paquier @ 2024-11-29 05:06 UTC (permalink / raw)
To: Michail Nikolaev <[email protected]>; +Cc: pgsql-hackers; [email protected]; Heikki Linnakangas <[email protected]>
On Thu, Nov 28, 2024 at 09:04:36PM +0100, Michail Nikolaev wrote:
> I discovered that a similar issue was previously addressed by Heikki in
> commit [0], where installcheck was disabled for injection point tests.
> However, in the meson build configuration, this was only applied to
> regression tests - the isolation and TAP tests are still running during
> installcheck.
I fail to see how this is related? The original issue was that this
was impossible to run safely concurrently, but now we have the
facilities able to do so. There are a few cases where using a wait
point has limits, for example outside a transaction context for some
of the processes, but that has not really been an issue up to now.
> As demonstrated in the previously shared reproducer [1], even *local*
> injection points can cause backend crashes through unexpected side effects.
> Therefore, I propose extending the installcheck disable to cover both TAP
> and isolation tests as well.
>
> I've attached a patch implementing these changes.
@@ -426,6 +427,7 @@ InvalidateCatalogSnapshot(void)
pairingheap_remove(&RegisteredSnapshots, &CatalogSnapshot->ph_node);
CatalogSnapshot = NULL;
SnapshotResetXmin();
+ INJECTION_POINT("invalidate_catalog_snapshot_end");
[...]
+step s4_attach_locally { SELECT
injection_points_attach('invalidate_catalog_snapshot_end', 'wait'); }
[...]
#4 0x0000563f09d22f39 in ExceptionalCondition
(conditionName=0x563f0a072d00
"TransactionIdIsValid(proc->xid)", fileName=0x563f0a072a4a
"procarray.c", lineNumber=677) at assert.c:66
#5 0x0000563f096f0684 in ProcArrayEndTransaction
(proc=0x7fbd4d083ac0, latestXid=750) at procarray.c:677
#6 0x0000563f088c54f3 in AbortTransaction () at xact.c:2946
#7 0x0000563f088c758d in AbortCurrentTransactionInternal () at
#xact.c:3531
#8 0x0000563f088c72a6 in AbortCurrentTransaction () at xact.c:3449
#9 0x0000563f0979c0f7 in PostgresMain (dbname=0x563f0f128100
"isolation_regression", username=0x563f0f1280e8 "ioltas") at
postgres.c:4524
#10 0x0000563f0978a5e5 in BackendMain (startup_data=0x7ffdcf50cfa8 "",
startup_data_len=4) at backend_startup.c:107
#11 0x0000563f094d8613 in postmaster_child_launch
(child_type=B_BACKEND, child_slot=1,
startup_data=0x7ffdcf50cfa8 "", startup_data_len=4,
client_sock=0x7ffdcf50cfe0)
Isn't that pointing to an actual bug with serializable transactions?
What you are telling here is that there is a race condition where it
is possible to trigger an assertion failure when finishing a session
while another one is waiting on an invalidation, if there's in the mix
a read/write dependency error. Disabling the test hides the problem,
it does not fix it. And we should do the latter, not the former.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-11-29 05:06 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-10 19:30 [PATCH] Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE Tomas Vondra <[email protected]>
2024-11-25 19:38 Strange assertion in procarray.c Michail Nikolaev <[email protected]>
2024-11-27 02:27 ` Re: Strange assertion in procarray.c Michail Nikolaev <[email protected]>
2024-11-27 09:50 ` Re: Strange assertion in procarray.c Michail Nikolaev <[email protected]>
2024-11-28 20:04 ` Re: Strange assertion in procarray.c Michail Nikolaev <[email protected]>
2024-11-29 05:06 ` Re: Strange assertion in procarray.c Michael Paquier <[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