On Tue, May 24, 2016 at 03:01:13PM -0400, Tom Lane wrote:
> Also, I notice another problem in vac_truncate_clog() now that I'm looking
> at it: it's expecting that the pg_database datfrozenxid and datminmxid
> values will hold still while it's looking at them. Since
> vac_update_datfrozenxid updates those values in-place, there's a race
> condition against VACUUMs happening in other databases. We should fetch
> those values into local variables before doing the various tests inside
> the scan loop.
Commit 2d2e40e fixed the above. There's another problem just like it, one
layer lower. vac_update_datfrozenxid() has:
if (TransactionIdPrecedes(classForm->relfrozenxid,
newFrozenXid))
newFrozenXid = classForm->relfrozenxid;
classForm points to buffer memory, and vac_update_relstats() inplace-updates
the buffer. Like vac_truncate_clog(), we don't mind using an old value, but
those two lines must use the same value. The attached test case shows this
bug making datfrozenxid move ahead of relfrozenxid. The attached patch fixes
it. (I noticed this while finishing up patches for the heap_inplace_update
writer race in https://postgr.es/m/20231102030915.d3.nmi...@google.com.)
I audited other read-only use of inplace-updated fields. Others look safe,
because they hold rel locks that exclude VACUUM, or they make only
non-critical decisions. Still, let's change some to the load-once style, to
improve the chance of future copy/paste finding the safe style. I'm attaching
a patch for that, too. I didn't add "volatile", because I couldn't think of
how we'd care if the load moved earlier.
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Test datfrozenxid/relfrozenxid update race condition
Not for commit, mostly because the injection point won't make sense
after the fix; it would be obvious that the test is unlikely to detect
similar future bugs. (A commit-ready test would also need to remove
temporary WARNING messages added to clarify what's happening, and it
would need to hide outputs sensitive to exact XID consumption.)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index b589279..73f0245 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -53,9 +53,11 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/acl.h"
+#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/guc.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
@@ -1620,6 +1622,8 @@ vac_update_datfrozenxid(void)
while ((classTup = systable_getnext(scan)) != NULL)
{
Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup);
+ TransactionId before, after;
+ before = classForm->relfrozenxid;
/*
* Only consider relations able to hold unfrozen XIDs (anything
else
@@ -1662,7 +1666,11 @@ vac_update_datfrozenxid(void)
/* determine new horizon */
if (TransactionIdPrecedes(classForm->relfrozenxid,
newFrozenXid))
+ {
+ if (namestrcmp(&classForm->relname,
"pg_test_last_table") == 0)
+
INJECTION_POINT("between-relfrozenxid-reads");
newFrozenXid = classForm->relfrozenxid;
+ }
}
if (MultiXactIdIsValid(classForm->relminmxid))
@@ -1678,6 +1686,12 @@ vac_update_datfrozenxid(void)
if (MultiXactIdPrecedes(classForm->relminmxid,
newMinMulti))
newMinMulti = classForm->relminmxid;
}
+
+ after = classForm->relfrozenxid;
+ elog(WARNING, "done reading %s before=%u after=%u myxid=%u
myxmin=%u",
+ NameStr(classForm->relname),
+ before, after,
+ MyProc->xid, MyProc->xmin);
}
/* we're done with pg_class */
diff --git a/src/backend/utils/adt/lockfuncs.c
b/src/backend/utils/adt/lockfuncs.c
index 13009cc..bb2db58 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -17,8 +17,11 @@
#include "funcapi.h"
#include "miscadmin.h"
#include "storage/predicate_internals.h"
+#include "storage/proc.h"
+#include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
+#include "utils/wait_event.h"
/*
@@ -606,8 +609,9 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
*
* Check if specified PID is blocked by any of the PIDs listed in the second
* argument. Currently, this looks for blocking caused by waiting for
- * heavyweight locks or safe snapshots. We ignore blockage caused by PIDs
- * not directly under the isolationtester's control, eg autovacuum.
+ * injection points, heavyweight locks, or safe snapshots. We ignore blockage
+ * caused by PIDs not directly under the isolationtester's control, eg
+ * autovacuum.
*
* This is an undocumented function intended for use by the isolation tester,
* and may change in future releases as required for testing purposes.
@@ -615,8 +619,11 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS)
Datum
pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
{
+#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32
*)&(var))))
int blocked_pid = PG_GETARG_INT32(0);
ArrayType *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1);
+ PGPROC *proc;
+ const char *wait_event;
ArrayType *blocking_pids_a;
int32 *interesting_pids;
int32 *blocking_pids;
@@ -626,6 +633,17 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
int i,
j;
+ /* Check if blocked_pid is in injection_wait(). */
+ proc = BackendPidGetProc(blocked_pid);
+ if (proc == NULL)
+ PG_RETURN_BOOL(false); /* session gone: definitely unblocked */
+ wait_event =
+
pgstat_get_wait_event(UINT32_ACCESS_ONCE(proc->wait_event_info));
+ if (wait_event && strncmp("INJECTION_POINT(",
+ wait_event,
+
strlen("INJECTION_POINT(")) == 0)
+ PG_RETURN_BOOL(true);
+
/* Validate the passed-in array */
Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID);
if (array_contains_nulls(interesting_pids_a))
diff --git a/src/test/modules/injection_points/Makefile
b/src/test/modules/injection_points/Makefile
index 31bd787..7ee6135d 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,6 +9,8 @@ PGFILEDESC = "injection_points - facility for injection points"
REGRESS = injection_points
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
+ISOLATION = inplace_update_reader
+
# The injection points are cluster-wide, so disable installcheck
NO_INSTALLCHECK = 1
diff --git
a/src/test/modules/injection_points/expected/inplace_update_reader.out
b/src/test/modules/injection_points/expected/inplace_update_reader.out
new file mode 100644
index 0000000..799100e
--- /dev/null
+++ b/src/test/modules/injection_points/expected/inplace_update_reader.out
@@ -0,0 +1,292 @@
+Parsed test spec with 5 sessions
+
+starting permutation: assign1 lock2 vac4 assign1 lock3 attach4 vac4 r2 vac5
detach1 read1 r3
+step assign1: SELECT txid_current();
+txid_current
+------------
+ 743
+(1 row)
+
+step lock2: BEGIN; LOCK pg_test_last_table IN SHARE UPDATE EXCLUSIVE MODE;
+s4: WARNING: done reading pg_statistic before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_type before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_toast_1255 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1247 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2604 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2606 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2612 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2600 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2619 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3381 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3429 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2618 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2620 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3466 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2609 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_foreign_table before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2615 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1262 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2964 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1213 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1260 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2396 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3600 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3079 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_2328 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1417 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_1418 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3118 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3256 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_6000 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_826 before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_toast_3394 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3596 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3592 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3456 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_6243 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_3350 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_6106 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_6100 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_authid before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_statistic_ext_data before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_user_mapping before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_subscription before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_attribute before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_proc before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_attrdef before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_constraint before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_inherits before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_index before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_operator before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_opfamily before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_opclass before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_am before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_amop before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_amproc before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_language before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_largeobject_metadata before=744 after=744
myxid=0 myxmin=744
+s4: WARNING: done reading pg_aggregate before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_statistic_ext before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_rewrite before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_trigger before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_event_trigger before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_description before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_cast before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_enum before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_namespace before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_conversion before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_depend before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_database before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_db_role_setting before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_tablespace before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_auth_members before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_shdepend before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_shdescription before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_ts_config before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_ts_config_map before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_ts_dict before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_ts_parser before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_ts_template before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_extension before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_foreign_data_wrapper before=744 after=744
myxid=0 myxmin=744
+s4: WARNING: done reading pg_foreign_server before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_policy before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_replication_origin before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_default_acl before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_init_privs before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_seclabel before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_shseclabel before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_collation before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_parameter_acl before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_partitioned_table before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_range before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_transform before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_sequence before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_publication before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_publication_namespace before=744 after=744
myxid=0 myxmin=744
+s4: WARNING: done reading pg_publication_rel before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_subscription_rel before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_largeobject before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_toast_14108 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading sql_features before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_toast_14113 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading sql_implementation_info before=744 after=744
myxid=0 myxmin=744
+s4: WARNING: done reading pg_toast_14118 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading sql_parts before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_toast_14123 before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading sql_sizing before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading pg_class before=744 after=744 myxid=0 myxmin=744
+s4: WARNING: done reading second_oldest before=744 after=744 myxid=0
myxmin=744
+s4: WARNING: done reading pg_test_last_table before=742 after=742 myxid=0
myxmin=744
+step vac4: VACUUM (FREEZE, SKIP_LOCKED);
+step assign1: SELECT txid_current();
+txid_current
+------------
+ 744
+(1 row)
+
+step lock3: BEGIN; LOCK second_oldest IN SHARE UPDATE EXCLUSIVE MODE;
+step attach4:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('between-relfrozenxid-reads', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+s4: WARNING: done reading pg_statistic before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_type before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_toast_1255 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1247 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2604 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2606 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2612 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2600 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2619 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3381 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3429 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2618 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2620 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3466 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2609 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_foreign_table before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2615 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1262 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2964 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1213 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1260 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2396 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3600 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3079 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_2328 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1417 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_1418 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3118 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3256 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_6000 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_826 before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_toast_3394 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3596 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3592 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3456 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_6243 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_3350 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_6106 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_6100 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_authid before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_statistic_ext_data before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_user_mapping before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_subscription before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_attribute before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_proc before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_attrdef before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_constraint before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_inherits before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_index before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_operator before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_opfamily before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_opclass before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_am before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_amop before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_amproc before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_language before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_largeobject_metadata before=745 after=745
myxid=0 myxmin=745
+s4: WARNING: done reading pg_aggregate before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_statistic_ext before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_rewrite before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_trigger before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_event_trigger before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_description before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_cast before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_enum before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_namespace before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_conversion before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_depend before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_database before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_db_role_setting before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_tablespace before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_auth_members before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_shdepend before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_shdescription before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_ts_config before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_ts_config_map before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_ts_dict before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_ts_parser before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_ts_template before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_extension before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_foreign_data_wrapper before=745 after=745
myxid=0 myxmin=745
+s4: WARNING: done reading pg_foreign_server before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_policy before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_replication_origin before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_default_acl before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_init_privs before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_seclabel before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_shseclabel before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_collation before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_parameter_acl before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_partitioned_table before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_range before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_transform before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_sequence before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_publication before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_publication_namespace before=745 after=745
myxid=0 myxmin=745
+s4: WARNING: done reading pg_publication_rel before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_subscription_rel before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_largeobject before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading pg_toast_14108 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading sql_features before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_toast_14113 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading sql_implementation_info before=745 after=745
myxid=0 myxmin=745
+s4: WARNING: done reading pg_toast_14118 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading sql_parts before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_toast_14123 before=745 after=745 myxid=0
myxmin=745
+s4: WARNING: done reading sql_sizing before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading pg_class before=745 after=745 myxid=0 myxmin=745
+s4: WARNING: done reading second_oldest before=744 after=744 myxid=0
myxmin=745
+step vac4: VACUUM (FREEZE, SKIP_LOCKED); <waiting ...>
+step r2: ROLLBACK;
+step vac5: VACUUM (FREEZE, SKIP_DATABASE_STATS) pg_test_last_table;
+step detach1:
+ SELECT injection_points_detach('between-relfrozenxid-reads');
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+s4: WARNING: done reading pg_test_last_table before=742 after=745 myxid=0
myxmin=745
+step vac4: <... completed>
+step read1:
+ -- should never return rows, but it does in this test case
+ select c.oid::regclass
+ from pg_database d, pg_class c
+ where relkind = 'r' and age(relfrozenxid) > age(datfrozenxid);
+
+ select age(datfrozenxid) from pg_database where datname = current_catalog;
+
+ select min(age(relfrozenxid)), max(age(relfrozenxid))
+ from pg_class c
+ where relkind = 'r';
+
+oid
+-------------
+second_oldest
+(1 row)
+
+age
+---
+ 0
+(1 row)
+
+min|max
+---+---
+ 0| 1
+(1 row)
+
+step r3: ROLLBACK;
diff --git a/src/test/modules/injection_points/injection_points.c
b/src/test/modules/injection_points/injection_points.c
index ace386d..f4fe678 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -205,6 +205,7 @@ injection_notice(const char *name)
void
injection_wait(const char *name)
{
+ char event_name[NAMEDATALEN];
uint32 old_wait_counts = 0;
int index = -1;
uint32 injection_wait_event = 0;
@@ -216,11 +217,11 @@ injection_wait(const char *name)
return;
/*
- * Use the injection point name for this custom wait event. Note that
- * this custom wait event name is not released, but we don't care much
for
- * testing as this should be short-lived.
+ * Note that this custom wait event name is not released, but we don't
+ * care much for testing as this should be short-lived.
*/
- injection_wait_event = WaitEventExtensionNew(name);
+ snprintf(event_name, sizeof(event_name), "INJECTION_POINT(%s)", name);
+ injection_wait_event = WaitEventExtensionNew(event_name);
/*
* Find a free slot to wait for, and register this injection point's
name.
@@ -402,14 +403,28 @@ Datum
injection_points_detach(PG_FUNCTION_ARGS)
{
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ int wait_index = -1;
InjectionPointDetach(name);
if (inj_state == NULL)
injection_init_shmem();
- /* Clean up any conditions associated to this injection point */
SpinLockAcquire(&inj_state->lock);
+
+ /* First bump any wait counter for the injection point to wake up */
+ for (int i = 0; i < INJ_MAX_WAIT; i++)
+ {
+ if (strcmp(name, inj_state->name[i]) == 0)
+ {
+ wait_index = i;
+ break;
+ }
+ }
+ if (wait_index >= 0)
+ inj_state->wait_counts[wait_index]++;
+
+ /* Clean up any conditions associated to this injection point */
for (int i = 0; i < INJ_MAX_CONDITION; i++)
{
InjectionPointCondition *condition = &inj_state->conditions[i];
@@ -420,7 +435,12 @@ injection_points_detach(PG_FUNCTION_ARGS)
condition->name[0] = '\0';
}
}
+
SpinLockRelease(&inj_state->lock);
+ /* And broadcast the change to any waiters */
+ if (wait_index >= 0)
+ ConditionVariableBroadcast(&inj_state->wait_point);
+
PG_RETURN_VOID();
}
diff --git a/src/test/modules/injection_points/specs/inplace_update_reader.spec
b/src/test/modules/injection_points/specs/inplace_update_reader.spec
new file mode 100644
index 0000000..325ba12
--- /dev/null
+++ b/src/test/modules/injection_points/specs/inplace_update_reader.spec
@@ -0,0 +1,75 @@
+# Test race between vac_update_relstats() and vac_update_datfrozenxid()
+
+# damage is hard to reach because:
+# - When v_u_d() first reads the value, the value must be a new minimum.
+# - The same v_u_d() must not read any later tuple that wipes out the damage.
+
+setup
+{
+ CREATE EXTENSION injection_points;
+}
+setup
+{
+ VACUUM FULL pg_class; -- help CREATE TABLE place at end
+}
+setup
+{
+ CREATE TABLE second_oldest ();
+ CREATE TABLE pg_test_last_table ();
+}
+
+teardown
+{
+ DROP EXTENSION injection_points;
+}
+
+
+# s1 does non-blocking, no-transaction-block activities
+session s1
+step assign1 { SELECT txid_current(); }
+step read1
+{
+ -- should never return rows, but it does in this test case
+ select c.oid::regclass
+ from pg_database d, pg_class c
+ where relkind = 'r' and age(relfrozenxid) > age(datfrozenxid);
+
+ select age(datfrozenxid) from pg_database where datname = current_catalog;
+
+ select min(age(relfrozenxid)), max(age(relfrozenxid))
+ from pg_class c
+ where relkind = 'r';
+}
+step detach1
+{
+ SELECT injection_points_detach('between-relfrozenxid-reads');
+}
+
+# s2 holds the lock on pg_test_last_table
+session s2
+step lock2 { BEGIN; LOCK pg_test_last_table IN SHARE UPDATE EXCLUSIVE
MODE; }
+step r2 { ROLLBACK; }
+
+# s3 holds the lock on second_oldest
+session s3
+step lock3 { BEGIN; LOCK second_oldest IN SHARE UPDATE EXCLUSIVE MODE; }
+step r3 { ROLLBACK; }
+
+# s4 does database-wide VACUUM w/ SKIP_LOCKED
+session s4
+step attach4
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('between-relfrozenxid-reads', 'wait');
+}
+step vac4 { VACUUM (FREEZE, SKIP_LOCKED); }
+
+# s5 does single-table VACUUM
+session s5
+step vac5 { VACUUM (FREEZE, SKIP_DATABASE_STATS) pg_test_last_table; }
+
+
+# 1st vac4 updates every relfrozenxid except pg_test_last_table
+# 2nd vac4 updates every relfrozenxid except pg_test_last_table and
second_oldest
+# vac5 updates pg_test_last_table relfrozenxid between the two
vac_update_datfrozenxid reads
+permutation assign1 lock2 vac4 assign1 lock3 attach4 vac4(detach1) r2 vac5
detach1 read1 r3
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Close race condition between datfrozen and relfrozen updates.
vac_update_datfrozenxid() did multiple loads of relfrozenxid and
relminmxid from buffer memory, and it assumed each would get the same
value. Not so if a concurrent vac_update_relstats() did an inplace
update. Commit 2d2e40e3befd8b9e0d2757554537345b15fa6ea2 fixed the same
kind of bug in vac_truncate_clog(). Today's bug could cause the
rel-level field and XIDs in the rel's rows to precede the db-level
field. A cluster having such values should VACUUM affected tables.
Back-patch to v12 (all supported versions).
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index b589279..a63a71c 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1611,6 +1611,8 @@ vac_update_datfrozenxid(void)
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
* index that can help us here.
+ *
+ * See vac_truncate_clog() for the race condition to prevent.
*/
relation = table_open(RelationRelationId, AccessShareLock);
@@ -1619,7 +1621,9 @@ vac_update_datfrozenxid(void)
while ((classTup = systable_getnext(scan)) != NULL)
{
- Form_pg_class classForm = (Form_pg_class) GETSTRUCT(classTup);
+ volatile FormData_pg_class *classForm = (Form_pg_class)
GETSTRUCT(classTup);
+ TransactionId relfrozenxid = classForm->relfrozenxid;
+ TransactionId relminmxid = classForm->relminmxid;
/*
* Only consider relations able to hold unfrozen XIDs (anything
else
@@ -1629,8 +1633,8 @@ vac_update_datfrozenxid(void)
classForm->relkind != RELKIND_MATVIEW &&
classForm->relkind != RELKIND_TOASTVALUE)
{
- Assert(!TransactionIdIsValid(classForm->relfrozenxid));
- Assert(!MultiXactIdIsValid(classForm->relminmxid));
+ Assert(!TransactionIdIsValid(relfrozenxid));
+ Assert(!MultiXactIdIsValid(relminmxid));
continue;
}
@@ -1649,34 +1653,34 @@ vac_update_datfrozenxid(void)
* before those relations have been scanned and cleaned up.
*/
- if (TransactionIdIsValid(classForm->relfrozenxid))
+ if (TransactionIdIsValid(relfrozenxid))
{
- Assert(TransactionIdIsNormal(classForm->relfrozenxid));
+ Assert(TransactionIdIsNormal(relfrozenxid));
/* check for values in the future */
- if (TransactionIdPrecedes(lastSaneFrozenXid,
classForm->relfrozenxid))
+ if (TransactionIdPrecedes(lastSaneFrozenXid,
relfrozenxid))
{
bogus = true;
break;
}
/* determine new horizon */
- if (TransactionIdPrecedes(classForm->relfrozenxid,
newFrozenXid))
- newFrozenXid = classForm->relfrozenxid;
+ if (TransactionIdPrecedes(relfrozenxid, newFrozenXid))
+ newFrozenXid = relfrozenxid;
}
- if (MultiXactIdIsValid(classForm->relminmxid))
+ if (MultiXactIdIsValid(relminmxid))
{
/* check for values in the future */
- if (MultiXactIdPrecedes(lastSaneMinMulti,
classForm->relminmxid))
+ if (MultiXactIdPrecedes(lastSaneMinMulti, relminmxid))
{
bogus = true;
break;
}
/* determine new horizon */
- if (MultiXactIdPrecedes(classForm->relminmxid,
newMinMulti))
- newMinMulti = classForm->relminmxid;
+ if (MultiXactIdPrecedes(relminmxid, newMinMulti))
+ newMinMulti = relminmxid;
}
}
Author: Noah Misch <n...@leadboat.com>
Commit: Noah Misch <n...@leadboat.com>
Avoid repeating loads of frozen ID values.
Repeating loads of inplace-updated fields tends to cause bugs like the
one from the previous commit. While there's no bug to fix in these code
sites, adopt the load-once style. This improves the chance of future
copy/paste finding the safe style.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4a4cf76..f14f6c2 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5907,7 +5907,6 @@ heap_abort_speculative(Relation relation, ItemPointer tid)
Page page;
BlockNumber block;
Buffer buffer;
- TransactionId prune_xid;
Assert(ItemPointerIsValid(tid));
@@ -5960,11 +5959,16 @@ heap_abort_speculative(Relation relation, ItemPointer
tid)
* TransactionXmin, so there's no race here).
*/
Assert(TransactionIdIsValid(TransactionXmin));
- if (TransactionIdPrecedes(TransactionXmin,
relation->rd_rel->relfrozenxid))
- prune_xid = relation->rd_rel->relfrozenxid;
- else
- prune_xid = TransactionXmin;
- PageSetPrunable(page, prune_xid);
+ {
+ TransactionId relfrozenxid = relation->rd_rel->relfrozenxid;
+ TransactionId prune_xid;
+
+ if (TransactionIdPrecedes(TransactionXmin, relfrozenxid))
+ prune_xid = relfrozenxid;
+ else
+ prune_xid = TransactionXmin;
+ PageSetPrunable(page, prune_xid);
+ }
/* store transaction information of xact deleting the tuple */
tp.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index c04886c..78f9678 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -919,18 +919,24 @@ copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid
OIDOldIndex, bool verbose,
* FreezeXid will become the table's new relfrozenxid, and that mustn't
go
* backwards, so take the max.
*/
- if (TransactionIdIsValid(OldHeap->rd_rel->relfrozenxid) &&
- TransactionIdPrecedes(cutoffs.FreezeLimit,
-
OldHeap->rd_rel->relfrozenxid))
- cutoffs.FreezeLimit = OldHeap->rd_rel->relfrozenxid;
+ {
+ TransactionId relfrozenxid = OldHeap->rd_rel->relfrozenxid;
+
+ if (TransactionIdIsValid(relfrozenxid) &&
+ TransactionIdPrecedes(cutoffs.FreezeLimit,
relfrozenxid))
+ cutoffs.FreezeLimit = relfrozenxid;
+ }
/*
* MultiXactCutoff, similarly, shouldn't go backwards either.
*/
- if (MultiXactIdIsValid(OldHeap->rd_rel->relminmxid) &&
- MultiXactIdPrecedes(cutoffs.MultiXactCutoff,
-
OldHeap->rd_rel->relminmxid))
- cutoffs.MultiXactCutoff = OldHeap->rd_rel->relminmxid;
+ {
+ MultiXactId relminmxid = OldHeap->rd_rel->relminmxid;
+
+ if (MultiXactIdIsValid(relminmxid) &&
+ MultiXactIdPrecedes(cutoffs.MultiXactCutoff,
relminmxid))
+ cutoffs.MultiXactCutoff = relminmxid;
+ }
/*
* Decide whether to use an indexscan or seqscan-and-optional-sort to
scan
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index a63a71c..521ee74 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1200,7 +1200,7 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams
*params,
aggressiveXIDCutoff = nextXID - freeze_table_age;
if (!TransactionIdIsNormal(aggressiveXIDCutoff))
aggressiveXIDCutoff = FirstNormalTransactionId;
- if (TransactionIdPrecedesOrEquals(rel->rd_rel->relfrozenxid,
+ if (TransactionIdPrecedesOrEquals(cutoffs->relfrozenxid,
aggressiveXIDCutoff))
return true;
@@ -1221,7 +1221,7 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams
*params,
aggressiveMXIDCutoff = nextMXID - multixact_freeze_table_age;
if (aggressiveMXIDCutoff < FirstMultiXactId)
aggressiveMXIDCutoff = FirstMultiXactId;
- if (MultiXactIdPrecedesOrEquals(rel->rd_rel->relminmxid,
+ if (MultiXactIdPrecedesOrEquals(cutoffs->relminmxid,
aggressiveMXIDCutoff))
return true;
diff --git a/src/backend/postmaster/autovacuum.c
b/src/backend/postmaster/autovacuum.c
index c367ede..9a925a1 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2938,6 +2938,7 @@ relation_needs_vacanalyze(Oid relid,
int freeze_max_age;
int multixact_freeze_max_age;
TransactionId xidForceLimit;
+ TransactionId relfrozenxid;
MultiXactId multiForceLimit;
Assert(classForm != NULL);
@@ -2989,16 +2990,18 @@ relation_needs_vacanalyze(Oid relid,
xidForceLimit = recentXid - freeze_max_age;
if (xidForceLimit < FirstNormalTransactionId)
xidForceLimit -= FirstNormalTransactionId;
- force_vacuum = (TransactionIdIsNormal(classForm->relfrozenxid) &&
-
TransactionIdPrecedes(classForm->relfrozenxid,
-
xidForceLimit));
+ relfrozenxid = classForm->relfrozenxid;
+ force_vacuum = (TransactionIdIsNormal(relfrozenxid) &&
+ TransactionIdPrecedes(relfrozenxid,
xidForceLimit));
if (!force_vacuum)
{
+ MultiXactId relminmxid = classForm->relminmxid;
+
multiForceLimit = recentMulti - multixact_freeze_max_age;
if (multiForceLimit < FirstMultiXactId)
multiForceLimit -= FirstMultiXactId;
- force_vacuum = MultiXactIdIsValid(classForm->relminmxid) &&
- MultiXactIdPrecedes(classForm->relminmxid,
multiForceLimit);
+ force_vacuum = MultiXactIdIsValid(relminmxid) &&
+ MultiXactIdPrecedes(relminmxid, multiForceLimit);
}
*wraparound = force_vacuum;
