Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sGrW4-000Pik-H8 for pgsql-hackers@arkaria.postgresql.org; Tue, 11 Jun 2024 02:45:45 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1sGrW1-001MFf-Np for pgsql-hackers@arkaria.postgresql.org; Tue, 11 Jun 2024 02:45:42 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1sGrW0-001MEl-M1 for pgsql-hackers@lists.postgresql.org; Tue, 11 Jun 2024 02:45:42 +0000 Received: from mail-ot1-x330.google.com ([2607:f8b0:4864:20::330]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1sGrVr-000j1n-DN for pgsql-hackers@postgresql.org; Tue, 11 Jun 2024 02:45:39 +0000 Received: by mail-ot1-x330.google.com with SMTP id 46e09a7af769-6f124e97d31so241145a34.1 for ; Mon, 10 Jun 2024 19:45:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=leadboat.com; s=google; t=1718073930; x=1718678730; darn=postgresql.org; h=user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=wRtfUbe9p14b2KFckeOozGy6l7j7ZcNsDvt2JmrAdBo=; b=dsvsU6dx8T9vEMYfA//CuaSJWhPVqbfI5sZsa8c5Kv9NMpLIcitL7eROMn8wgW7yAL VvHupDVVWf/8Boma8ppWOLdUC8lxwqXIii5byqCvxRILUmDnrZtpJyvH2bJxjAGQw0Ae xD84WmPMayo/3mzXFeiSkEodVvbgNg6kcWoNI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718073930; x=1718678730; h=user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=wRtfUbe9p14b2KFckeOozGy6l7j7ZcNsDvt2JmrAdBo=; b=ELdKxKXEDy06Y0ALvPA7IQWYBH+gVILKWfHiPVMiAlOBkuKGq0FuZ6ZP9VkAArUeHf vhnqTb4ieYJQuz+c/N/kO/AGRifjha5L7AzF5u/wY3XICjheJn3TBuV/+zXJWxBkXjHl V/36vui9WfrB9lwLLIdJ43RuDCbg+yA34AeiRMMmmS4kk0wal+/27by4DuX3bbw2tTsO 5E3lu7sZgJxPTCdcNoAZRG0hPElO0wu/G/O0BSBwkLF/cZD6+pXLVR1EfYcC11X54WLO mPngtW0ggjLZ8iAIDkT3U6gMAnXX1VoxVwNRoU7eGOKuY6jcF2ubGV+Y9sFsNgvlSjmY ZA/g== X-Gm-Message-State: AOJu0YxOX/xR6lomJJKodWQr8LJi9iIUqJG09qogW+FzX6sXEollvM5B XQwx69xyjDccjKtANPLnfHbeGDD5g+iBhjnoCAR5DKf6aMLbPPuDcv1PO18Uhg== X-Google-Smtp-Source: AGHT+IHex4UiEl+cmA1ad5IB9Gw58PG5VvRdZOvk3tKETBvOW8O6gyRzAsbSGCTr8UPeTW71/xbFrQ== X-Received: by 2002:a9d:6f83:0:b0:6f9:7bef:796a with SMTP id 46e09a7af769-6f97bef80d2mr8120005a34.21.1718073930093; Mon, 10 Jun 2024 19:45:30 -0700 (PDT) Received: from google.com ([2600:1702:a20:5750::48]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-6f96367603dsm1339977a34.53.2024.06.10.19.45.27 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 10 Jun 2024 19:45:28 -0700 (PDT) Date: Mon, 10 Jun 2024 19:45:25 -0700 From: Noah Misch To: Robert Haas Cc: pgsql-hackers@postgresql.org, Tom Lane , Smolkin Grigory , Michael Paquier Subject: Re: race condition in pg_class Message-ID: <20240611024525.9f.nmisch@google.com> References: <20231027044404.7e.nmisch@google.com> <20231027184832.9b.nmisch@google.com> <1596629.1698435146@sss.pgh.pa.us> <20231027214946.79.nmisch@google.com> <1617596.1698446455@sss.pgh.pa.us> <20231027232612.99.nmisch@google.com> <20231102030915.d3.nmisch@google.com> <20240512232923.aa.nmisch@google.com> <20240605181706.fe.nmisch@google.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="3pz3+3yBgJC2Q0yV" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/2.2.12 (2023-09-09) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit On Thu, Jun 06, 2024 at 09:48:51AM -0400, Robert Haas wrote: > On Wed, Jun 5, 2024 at 2:17 PM Noah Misch wrote: > > Starting 2024-06-10, I plan to push the first seven of the ten patches: > > > > inplace005-UNEXPECTEDPASS-tap-meson-v1.patch > > inplace010-tests-v1.patch > > inplace040-waitfuncs-v1.patch > > inplace050-tests-inj-v1.patch > > inplace060-nodeModifyTable-comments-v1.patch > > Those five just deal in tests, test infrastructure, and comments. > > inplace070-rel-locks-missing-v1.patch > > Main risk is new DDL deadlocks. > > inplace080-catcache-detoast-inplace-stale-v1.patch > > If it fails to fix the bug it targets, I expect it's a no-op rather than > > breaking things. > > > > I'll leave the last three of the ten needing review. Those three are beyond > > my skill to self-certify. > > It's not this patch set's fault, but I'm not very pleased to see that > the injection point wait events have been shoehorned into the > "Extension" category I've replied on that branch of the thread. > I think that the comments and commit messages in this patch set could, > in some places, use improvement. For instance, > inplace060-nodeModifyTable-comments-v1.patch reflows a bunch of > comments, which makes it hard to see what actually changed, and the > commit message doesn't tell you, either. A good bit of it seems to be > changing "a view" to "a view INSTEAD OF trigger" or "a view having an > INSTEAD OF trigger," but the reasoning behind that change is not > spelled out anywhere. The reader is left to guess what the other case > is and why the same principles don't apply to it. I don't doubt that > the new comments are more correct than the old ones, but I expect > future patch authors to have difficulty maintaining that state of > affairs. The two kinds are trigger-updatable views and auto-updatable views. I've added sentences about that to the nodeModifyTable.c header comment. One could argue for dropping the INSTEAD OF comment changes outside of the header. > Similarly, inplace070-rel-locks-missing-v1.patch adds no comments. > IMHO, the commit message also isn't very informative. It disclaims > knowledge of what bug it's fixing, while at the same time leaving the > reader to figure out for themselves how the behavior has changed. > Consequently, I expect writing the release notes for a release > including this patch to be difficult: "We added some locks that block > ... something ... in some circumstances ... to prevent ... something." > It's not really the job of the release note author to fill in those > blanks, but rather of the patch author or committer. I don't want to I had been thinking release notes should just say "Add missing DDL lock acquisitions". One can cure a breach of our locking standards without proving some specific bad outcome. However, one could counter that commands like GRANT follow a different standard, and perhaps SetRelationHasSubclass() should use the GRANT standard. Hence, I researched the bugs this fixes and split inplace070-rel-locks-missing into three patches: 1. [inplace065-lock-SequenceChangePersistence] Lock in SequenceChangePersistence(), where the omission can lose nextval() increments of the sequence. 2. [inplace071-lock-SetRelationHasSubclass] Lock in SetRelationHasSubclass(). This one has only minor benefits; see the new commit message. A fair alternative would be tuple-level locking in inplace120-locktag, like that patch adds to GRANT. That might avoid some deadlocks. I feel like the minor benefits justify the way I chose, but it's a weak preference. 3. [inplace075-lock-heap_create] Add to heap creation: > overburden the act of fixing bugs, but I just feel like more > explanation is needed here. When I see for example that we're adding a > lock acquisition to the end of heap_create(), I can't help but wonder > if it's really true that we don't take a lock on a just-created > relation today. I'm certainly under the impression that we lock > newly-created, uncommitted relations, and a quick test seems to > confirm that. I don't quite know whether that happens, but evidently > this call is guarding against something more subtle than a categorical > failure to lock a relation on creation so I think there should be a > comment explaining what that thing is. I've covered that in the new log message. To lock as early as possible, I've moved this up a layer, to just after relid assignment. One could argue this change belongs in inplace120 rather than its own patch, since it's only here to eliminate a harmless exception to the rule inplace120 asserts. I've removed the update_relispartition() that appeared in inplace070-rel-locks-missing-v1.patch. Only an older, unpublished draft of the rules (that inplace110-successors adds to README.tuplock) required that lock. The lock might be worthwhile for avoiding "tuple concurrently updated", but it's out of scope for $SUBJECT. > It's also quite surprising that SetRelationHasSubclass() says "take X > lock before calling" and 2 of 4 callers just don't. I guess that's how > it is. But shouldn't we then have an assertion inside that function to > guard against future mistakes? If the reason why we failed to add this Works for me. Done. I've moved the LockHeldByMe() change from inplace110-successors to this patch, since the assertion wants it. > initially is discernible from the commit messages that introduced the > bug, it would be nice to mention what it seems to have been; if not, > it would at least be nice to mention the offending commit(s). I'm also Done. > a bit worried that this is going to cause deadlocks, but I suppose if > it does, that's still better than the status quo. > > IsInplaceUpdateOid's header comment says IsInplaceUpdateRelation > instead of IsInplaceUpdateOid. Fixed. > inplace080-catcache-detoast-inplace-stale-v1.patch seems like another > place where spelling out the rationale in more detail would be helpful > to future readers; for instance, the commit message says that > PgDatabaseToastTable is the only one affected, but it doesn't say why > the others are not, or why this one is. The lengthy comment in I've updated the commit message to answer that. > CatalogCacheCreateEntry is also difficult to correlate with the code > which follows. I can't guess whether the two cases called out in the > comment always needed to be handled and were handled save only for > in-place updates, and thus the comment changes were simply taking the > opportunity to elaborate on the existing comments; or whether one of > those cases is preexisting and the other arises from the desire to > handle inplace updates. It could be helpful to mention relevant > identifiers from the code in the comment text e.g. > "systable_recheck_tuple detects ordinary updates by noting changes to > the tuple's visibility information, while the equalTuple() case > detects inplace updates." The patch was elaborating on existing comments. Reading the patch again today, the elaboration no longer feels warranted. Hence, I've rewritten that comment addition. I've included identifiers, and the patch no longer adds comment material orthogonal to inplace updates. Thanks, nm --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace005-UNEXPECTEDPASS-tap-meson-v2.patch" Author: Noah Misch Commit: Noah Misch Make TAP todo_start effects the same under Meson and prove_check. This could have caused spurious failures only on SPARC Linux, because today's only todo_start tests for that platform. Back-patch to v16, where Meson support first appeared. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/tools/testwrap b/src/tools/testwrap index d01e610..9a270be 100755 --- a/src/tools/testwrap +++ b/src/tools/testwrap @@ -41,12 +41,22 @@ env_dict = {**os.environ, 'TESTDATADIR': os.path.join(testdir, 'data'), 'TESTLOGDIR': os.path.join(testdir, 'log')} -sp = subprocess.run(args.test_command, env=env_dict) +sp = subprocess.Popen(args.test_command, env=env_dict, stdout=subprocess.PIPE) +# Meson categorizes a passing TODO test point as bad +# (https://github.com/mesonbuild/meson/issues/13183). Remove the TODO +# directive, so Meson computes the file result like Perl does. This could +# have the side effect of delaying stdout lines relative to stderr. That +# doesn't affect the log file, and the TAP protocol uses stdout only. +for line in sp.stdout: + if line.startswith(b'ok '): + line = line.replace(b' # TODO ', b' # testwrap-overridden-TODO ', 1) + sys.stdout.buffer.write(line) +returncode = sp.wait() -if sp.returncode == 0: +if returncode == 0: print('# test succeeded') open(os.path.join(testdir, 'test.success'), 'x') else: print('# test failed') open(os.path.join(testdir, 'test.fail'), 'x') -sys.exit(sp.returncode) +sys.exit(returncode) --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace010-tests-v2.patch" Author: Noah Misch Commit: Noah Misch Improve test coverage for changes to inplace-updated catalogs. This covers both regular and inplace changes, since bugs arise at their intersection. Where marked, these witness extant bugs. Back-patch to v12 (all supported versions). Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl index d0a86a2..4d5f8d2 100644 --- a/src/bin/pgbench/t/001_pgbench_with_server.pl +++ b/src/bin/pgbench/t/001_pgbench_with_server.pl @@ -68,6 +68,34 @@ $node->pgbench( "CREATE TYPE pg_temp.e AS ENUM ($labels); DROP TYPE pg_temp.e;" }); +# Test inplace updates from VACUUM concurrent with heap_update from GRANT. +# The PROC_IN_VACUUM environment can't finish MVCC table scans consistently, +# so this fails rarely. To reproduce consistently, add a sleep after +# GetCatalogSnapshot(non-catalog-rel). +Test::More->builder->todo_start('PROC_IN_VACUUM scan breakage'); +$node->safe_psql('postgres', 'CREATE TABLE ddl_target ()'); +$node->pgbench( + '--no-vacuum --client=5 --protocol=prepared --transactions=50', + 0, + [qr{processed: 250/250}], + [qr{^$}], + 'concurrent GRANT/VACUUM', + { + '001_pgbench_grant@9' => q( + DO $$ + BEGIN + PERFORM pg_advisory_xact_lock(42); + FOR i IN 1 .. 10 LOOP + GRANT SELECT ON ddl_target TO PUBLIC; + REVOKE SELECT ON ddl_target FROM PUBLIC; + END LOOP; + END + $$; +), + '001_pgbench_vacuum_ddl_target@1' => "VACUUM ddl_target;", + }); +Test::More->builder->todo_end; + # Trigger various connection errors $node->pgbench( 'no-such-database', diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index 0237271..032d420 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -1337,3 +1337,29 @@ a|b|c| d 2|2|2|1004 (2 rows) + +starting permutation: sys1 sysupd2 c1 c2 +step sys1: + UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass; + +step sysupd2: + UPDATE pg_class SET reltuples = reltuples * 2 + WHERE oid = 'accounts'::regclass; + +step c1: COMMIT; +step sysupd2: <... completed> +step c2: COMMIT; + +starting permutation: sys1 sysmerge2 c1 c2 +step sys1: + UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass; + +step sysmerge2: + MERGE INTO pg_class + USING (SELECT 'accounts'::regclass AS o) j + ON o = oid + WHEN MATCHED THEN UPDATE SET reltuples = reltuples * 2; + +step c1: COMMIT; +step sysmerge2: <... completed> +step c2: COMMIT; diff --git a/src/test/isolation/expected/inplace-inval.out b/src/test/isolation/expected/inplace-inval.out new file mode 100644 index 0000000..67b34ad --- /dev/null +++ b/src/test/isolation/expected/inplace-inval.out @@ -0,0 +1,32 @@ +Parsed test spec with 3 sessions + +starting permutation: cachefill3 cir1 cic2 ddl3 read1 +step cachefill3: TABLE newly_indexed; +c +- +(0 rows) + +step cir1: BEGIN; CREATE INDEX i1 ON newly_indexed (c); ROLLBACK; +step cic2: CREATE INDEX i2 ON newly_indexed (c); +step ddl3: ALTER TABLE newly_indexed ADD extra int; +step read1: + SELECT relhasindex FROM pg_class WHERE oid = 'newly_indexed'::regclass; + +relhasindex +----------- +f +(1 row) + + +starting permutation: cir1 cic2 ddl3 read1 +step cir1: BEGIN; CREATE INDEX i1 ON newly_indexed (c); ROLLBACK; +step cic2: CREATE INDEX i2 ON newly_indexed (c); +step ddl3: ALTER TABLE newly_indexed ADD extra int; +step read1: + SELECT relhasindex FROM pg_class WHERE oid = 'newly_indexed'::regclass; + +relhasindex +----------- +t +(1 row) + diff --git a/src/test/isolation/expected/intra-grant-inplace-db.out b/src/test/isolation/expected/intra-grant-inplace-db.out new file mode 100644 index 0000000..432ece5 --- /dev/null +++ b/src/test/isolation/expected/intra-grant-inplace-db.out @@ -0,0 +1,28 @@ +Parsed test spec with 3 sessions + +starting permutation: snap3 b1 grant1 vac2 snap3 c1 cmp3 +step snap3: + INSERT INTO frozen_witness + SELECT datfrozenxid FROM pg_database WHERE datname = current_catalog; + +step b1: BEGIN; +step grant1: + GRANT TEMP ON DATABASE isolation_regression TO regress_temp_grantee; + +step vac2: VACUUM (FREEZE); +step snap3: + INSERT INTO frozen_witness + SELECT datfrozenxid FROM pg_database WHERE datname = current_catalog; + +step c1: COMMIT; +step cmp3: + SELECT 'datfrozenxid retreated' + FROM pg_database + WHERE datname = current_catalog + AND age(datfrozenxid) > (SELECT min(age(x)) FROM frozen_witness); + +?column? +---------------------- +datfrozenxid retreated +(1 row) + diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out new file mode 100644 index 0000000..cc1e47a --- /dev/null +++ b/src/test/isolation/expected/intra-grant-inplace.out @@ -0,0 +1,225 @@ +Parsed test spec with 5 sessions + +starting permutation: b1 grant1 read2 addk2 c1 read2 +step b1: BEGIN; +step grant1: + GRANT SELECT ON intra_grant_inplace TO PUBLIC; + +step read2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step c1: COMMIT; +step read2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; + +relhasindex +----------- +f +(1 row) + + +starting permutation: keyshr5 addk2 +step keyshr5: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR KEY SHARE; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); + +starting permutation: keyshr5 b3 sfnku3 addk2 r3 +step keyshr5: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR KEY SHARE; + +relhasindex +----------- +f +(1 row) + +step b3: BEGIN ISOLATION LEVEL READ COMMITTED; +step sfnku3: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step r3: ROLLBACK; + +starting permutation: b2 sfnku2 addk2 c2 +step b2: BEGIN; +step sfnku2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step c2: COMMIT; + +starting permutation: keyshr5 b2 sfnku2 addk2 c2 +step keyshr5: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR KEY SHARE; + +relhasindex +----------- +f +(1 row) + +step b2: BEGIN; +step sfnku2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step c2: COMMIT; + +starting permutation: b3 sfu3 b1 grant1 read2 addk2 r3 c1 read2 +step b3: BEGIN ISOLATION LEVEL READ COMMITTED; +step sfu3: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR UPDATE; + +relhasindex +----------- +f +(1 row) + +step b1: BEGIN; +step grant1: + GRANT SELECT ON intra_grant_inplace TO PUBLIC; + +step read2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; + +relhasindex +----------- +f +(1 row) + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step r3: ROLLBACK; +step grant1: <... completed> +step c1: COMMIT; +step read2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; + +relhasindex +----------- +f +(1 row) + + +starting permutation: b2 sfnku2 b1 grant1 addk2 c2 c1 read2 +step b2: BEGIN; +step sfnku2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; + +relhasindex +----------- +f +(1 row) + +step b1: BEGIN; +step grant1: + GRANT SELECT ON intra_grant_inplace TO PUBLIC; + +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step c2: COMMIT; +step grant1: <... completed> +step c1: COMMIT; +step read2: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; + +relhasindex +----------- +f +(1 row) + + +starting permutation: b1 grant1 b3 sfu3 revoke4 c1 r3 +step b1: BEGIN; +step grant1: + GRANT SELECT ON intra_grant_inplace TO PUBLIC; + +step b3: BEGIN ISOLATION LEVEL READ COMMITTED; +step sfu3: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR UPDATE; + +step revoke4: + DO $$ + BEGIN + REVOKE SELECT ON intra_grant_inplace FROM PUBLIC; + EXCEPTION WHEN others THEN + RAISE WARNING 'got: %', regexp_replace(sqlerrm, '[0-9]+', 'REDACTED'); + END + $$; + +step c1: COMMIT; +step sfu3: <... completed> +relhasindex +----------- +f +(1 row) + +s4: WARNING: got: tuple concurrently updated +step revoke4: <... completed> +step r3: ROLLBACK; + +starting permutation: b1 drop1 b3 sfu3 revoke4 c1 r3 +step b1: BEGIN; +step drop1: + DROP TABLE intra_grant_inplace; + +step b3: BEGIN ISOLATION LEVEL READ COMMITTED; +step sfu3: + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR UPDATE; + +step revoke4: + DO $$ + BEGIN + REVOKE SELECT ON intra_grant_inplace FROM PUBLIC; + EXCEPTION WHEN others THEN + RAISE WARNING 'got: %', regexp_replace(sqlerrm, '[0-9]+', 'REDACTED'); + END + $$; + +step c1: COMMIT; +step sfu3: <... completed> +relhasindex +----------- +(0 rows) + +s4: WARNING: got: tuple concurrently deleted +step revoke4: <... completed> +step r3: ROLLBACK; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 0342eb3..6da98cf 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -37,6 +37,9 @@ test: fk-snapshot test: subxid-overflow test: eval-plan-qual test: eval-plan-qual-trigger +test: inplace-inval +test: intra-grant-inplace +test: intra-grant-inplace-db test: lock-update-delete test: lock-update-traversal test: inherit-temp diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index edd6d19..3a74406 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -194,6 +194,12 @@ step simplepartupdate_noroute { update parttbl set b = 2 where c = 1 returning *; } +# test system class updates + +step sys1 { + UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass; +} + session s2 setup { BEGIN ISOLATION LEVEL READ COMMITTED; } @@ -282,6 +288,18 @@ step wnested2 { ); } +step sysupd2 { + UPDATE pg_class SET reltuples = reltuples * 2 + WHERE oid = 'accounts'::regclass; +} + +step sysmerge2 { + MERGE INTO pg_class + USING (SELECT 'accounts'::regclass AS o) j + ON o = oid + WHEN MATCHED THEN UPDATE SET reltuples = reltuples * 2; +} + step c2 { COMMIT; } step r2 { ROLLBACK; } @@ -380,3 +398,6 @@ permutation simplepartupdate complexpartupdate c1 c2 read_part permutation simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 read_part permutation simplepartupdate_noroute complexpartupdate_route c1 c2 read_part permutation simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 read_part + +permutation sys1 sysupd2 c1 c2 +permutation sys1 sysmerge2 c1 c2 diff --git a/src/test/isolation/specs/inplace-inval.spec b/src/test/isolation/specs/inplace-inval.spec new file mode 100644 index 0000000..d8e1c98 --- /dev/null +++ b/src/test/isolation/specs/inplace-inval.spec @@ -0,0 +1,38 @@ +# If a heap_update() caller retrieves its oldtup from a cache, it's possible +# for that cache entry to predate an inplace update, causing loss of that +# inplace update. This arises because the transaction may abort before +# sending the inplace invalidation message to the shared queue. + +setup +{ + CREATE TABLE newly_indexed (c int); +} + +teardown +{ + DROP TABLE newly_indexed; +} + +session s1 +step cir1 { BEGIN; CREATE INDEX i1 ON newly_indexed (c); ROLLBACK; } +step read1 { + SELECT relhasindex FROM pg_class WHERE oid = 'newly_indexed'::regclass; +} + +session s2 +step cic2 { CREATE INDEX i2 ON newly_indexed (c); } + +session s3 +step cachefill3 { TABLE newly_indexed; } +step ddl3 { ALTER TABLE newly_indexed ADD extra int; } + + +permutation + cachefill3 # populates the pg_class row in the catcache + cir1 # sets relhasindex=true; rollback discards cache inval + cic2 # sees relhasindex=true, skips changing it (so no inval) + ddl3 # cached row as the oldtup of an update, losing relhasindex + read1 # observe damage XXX is an extant bug + +# without cachefill3, no bug +permutation cir1 cic2 ddl3 read1 diff --git a/src/test/isolation/specs/intra-grant-inplace-db.spec b/src/test/isolation/specs/intra-grant-inplace-db.spec new file mode 100644 index 0000000..bbecd5d --- /dev/null +++ b/src/test/isolation/specs/intra-grant-inplace-db.spec @@ -0,0 +1,46 @@ +# GRANT's lock is the catalog tuple xmax. GRANT doesn't acquire a heavyweight +# lock on the object undergoing an ACL change. In-place updates, namely +# datfrozenxid, need special code to cope. + +setup +{ + CREATE ROLE regress_temp_grantee; +} + +teardown +{ + REVOKE ALL ON DATABASE isolation_regression FROM regress_temp_grantee; + DROP ROLE regress_temp_grantee; +} + +# heap_update(pg_database) +session s1 +step b1 { BEGIN; } +step grant1 { + GRANT TEMP ON DATABASE isolation_regression TO regress_temp_grantee; +} +step c1 { COMMIT; } + +# inplace update +session s2 +step vac2 { VACUUM (FREEZE); } + +# observe datfrozenxid +session s3 +setup { + CREATE TEMP TABLE frozen_witness (x xid); +} +step snap3 { + INSERT INTO frozen_witness + SELECT datfrozenxid FROM pg_database WHERE datname = current_catalog; +} +step cmp3 { + SELECT 'datfrozenxid retreated' + FROM pg_database + WHERE datname = current_catalog + AND age(datfrozenxid) > (SELECT min(age(x)) FROM frozen_witness); +} + + +# XXX extant bug +permutation snap3 b1 grant1 vac2(c1) snap3 c1 cmp3 diff --git a/src/test/isolation/specs/intra-grant-inplace.spec b/src/test/isolation/specs/intra-grant-inplace.spec new file mode 100644 index 0000000..3cd696b --- /dev/null +++ b/src/test/isolation/specs/intra-grant-inplace.spec @@ -0,0 +1,153 @@ +# GRANT's lock is the catalog tuple xmax. GRANT doesn't acquire a heavyweight +# lock on the object undergoing an ACL change. Inplace updates, such as +# relhasindex=true, need special code to cope. + +setup +{ + CREATE TABLE intra_grant_inplace (c int); +} + +teardown +{ + DROP TABLE IF EXISTS intra_grant_inplace; +} + +# heap_update() +session s1 +step b1 { BEGIN; } +step grant1 { + GRANT SELECT ON intra_grant_inplace TO PUBLIC; +} +step drop1 { + DROP TABLE intra_grant_inplace; +} +step c1 { COMMIT; } + +# inplace update +session s2 +step read2 { + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass; +} +step b2 { BEGIN; } +step addk2 { ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); } +step sfnku2 { + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; +} +step c2 { COMMIT; } + +# rowmarks +session s3 +step b3 { BEGIN ISOLATION LEVEL READ COMMITTED; } +step sfnku3 { + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR NO KEY UPDATE; +} +step sfu3 { + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR UPDATE; +} +step r3 { ROLLBACK; } + +# Additional heap_update() +session s4 +# swallow error message to keep any OID value out of expected output +step revoke4 { + DO $$ + BEGIN + REVOKE SELECT ON intra_grant_inplace FROM PUBLIC; + EXCEPTION WHEN others THEN + RAISE WARNING 'got: %', regexp_replace(sqlerrm, '[0-9]+', 'REDACTED'); + END + $$; +} + +# Additional rowmarks +session s5 +setup { BEGIN; } +step keyshr5 { + SELECT relhasindex FROM pg_class + WHERE oid = 'intra_grant_inplace'::regclass FOR KEY SHARE; +} +teardown { ROLLBACK; } + + +# XXX extant bugs: permutation comments refer to planned post-bugfix behavior + +permutation + b1 + grant1 + read2 + addk2(c1) # inplace waits + c1 + read2 + +# inplace thru KEY SHARE +permutation + keyshr5 + addk2 + +# inplace wait NO KEY UPDATE w/ KEY SHARE +permutation + keyshr5 + b3 + sfnku3 + addk2(r3) + r3 + +# same-xact rowmark +permutation + b2 + sfnku2 + addk2 + c2 + +# same-xact rowmark in multixact +permutation + keyshr5 + b2 + sfnku2 + addk2 + c2 + +permutation + b3 + sfu3 + b1 + grant1(r3) # acquire LockTuple(), await sfu3 xmax + read2 + addk2(c1) # block in LockTuple() behind grant1 + r3 # unblock grant1; addk2 now awaits grant1 xmax + c1 + read2 + +permutation + b2 + sfnku2 + b1 + grant1(c2) # acquire LockTuple(), await sfnku2 xmax + addk2 # block in LockTuple() behind grant1 = deadlock + c2 + c1 + read2 + +# SearchSysCacheLocked1() calling LockRelease() +permutation + b1 + grant1 + b3 + sfu3(c1) # acquire LockTuple(), await grant1 xmax + revoke4(sfu3) # block in LockTuple() behind sfu3 + c1 + r3 # revoke4 unlocks old tuple and finds new + +# SearchSysCacheLocked1() finding a tuple, then no tuple +permutation + b1 + drop1 + b3 + sfu3(c1) # acquire LockTuple(), await drop1 xmax + revoke4(sfu3) # block in LockTuple() behind sfu3 + c1 # sfu3 locks none; revoke4 unlocks old and finds none + r3 diff --git a/src/test/regress/expected/database.out b/src/test/regress/expected/database.out new file mode 100644 index 0000000..30c0865 --- /dev/null +++ b/src/test/regress/expected/database.out @@ -0,0 +1,14 @@ +CREATE DATABASE regression_tbd ENCODING utf8 LOCALE "C" TEMPLATE template0; +ALTER DATABASE regression_tbd RENAME TO regression_utf8; +ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace; +ALTER DATABASE regression_utf8 RESET TABLESPACE; +ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123; +-- Test PgDatabaseToastTable. Doing this organically and portably would take +-- a huge relacl, which would be slow. +BEGIN; +UPDATE pg_database SET datcollversion = repeat('a', 6e6::int) +WHERE datname = 'regression_utf8'; +-- load catcache entry, if nothing else does +ALTER DATABASE regression_utf8 RESET TABLESPACE; +ROLLBACK; +DROP DATABASE regression_utf8; diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out index eddc1f4..3d33259 100644 --- a/src/test/regress/expected/merge.out +++ b/src/test/regress/expected/merge.out @@ -2691,6 +2691,30 @@ drop cascades to table measurement_y2007m01 DROP FUNCTION measurement_insert_trigger(); -- prepare RESET SESSION AUTHORIZATION; +-- try a system catalog +MERGE INTO pg_class c +USING (SELECT 'pg_depend'::regclass AS oid) AS j +ON j.oid = c.oid +WHEN MATCHED THEN + UPDATE SET reltuples = reltuples + 1 +RETURNING j.oid; + oid +----------- + pg_depend +(1 row) + +CREATE VIEW classv AS SELECT * FROM pg_class; +MERGE INTO classv c +USING pg_namespace n +ON n.oid = c.relnamespace +WHEN MATCHED AND c.oid = 'pg_depend'::regclass THEN + UPDATE SET reltuples = reltuples - 1 +RETURNING c.oid; + oid +------ + 2608 +(1 row) + DROP TABLE target, target2; DROP TABLE source, source2; DROP FUNCTION merge_trigfunc(); diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index 675c567..ddc155c 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -28,7 +28,7 @@ test: strings md5 numerology point lseg line box path polygon circle date time t # geometry depends on point, lseg, line, box, path, polygon, circle # horology depends on date, time, timetz, timestamp, timestamptz, interval # ---------- -test: geometry horology tstypes regex type_sanity opr_sanity misc_sanity comments expressions unicode xid mvcc +test: geometry horology tstypes regex type_sanity opr_sanity misc_sanity comments expressions unicode xid mvcc database # ---------- # Load huge amounts of data diff --git a/src/test/regress/sql/database.sql b/src/test/regress/sql/database.sql new file mode 100644 index 0000000..6c61f2e --- /dev/null +++ b/src/test/regress/sql/database.sql @@ -0,0 +1,16 @@ +CREATE DATABASE regression_tbd ENCODING utf8 LOCALE "C" TEMPLATE template0; +ALTER DATABASE regression_tbd RENAME TO regression_utf8; +ALTER DATABASE regression_utf8 SET TABLESPACE regress_tblspace; +ALTER DATABASE regression_utf8 RESET TABLESPACE; +ALTER DATABASE regression_utf8 CONNECTION_LIMIT 123; + +-- Test PgDatabaseToastTable. Doing this organically and portably would take +-- a huge relacl, which would be slow. +BEGIN; +UPDATE pg_database SET datcollversion = repeat('a', 6e6::int) +WHERE datname = 'regression_utf8'; +-- load catcache entry, if nothing else does +ALTER DATABASE regression_utf8 RESET TABLESPACE; +ROLLBACK; + +DROP DATABASE regression_utf8; diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql index 3d5d854..92163ec 100644 --- a/src/test/regress/sql/merge.sql +++ b/src/test/regress/sql/merge.sql @@ -1713,6 +1713,23 @@ DROP FUNCTION measurement_insert_trigger(); -- prepare RESET SESSION AUTHORIZATION; + +-- try a system catalog +MERGE INTO pg_class c +USING (SELECT 'pg_depend'::regclass AS oid) AS j +ON j.oid = c.oid +WHEN MATCHED THEN + UPDATE SET reltuples = reltuples + 1 +RETURNING j.oid; + +CREATE VIEW classv AS SELECT * FROM pg_class; +MERGE INTO classv c +USING pg_namespace n +ON n.oid = c.relnamespace +WHEN MATCHED AND c.oid = 'pg_depend'::regclass THEN + UPDATE SET reltuples = reltuples - 1 +RETURNING c.oid; + DROP TABLE target, target2; DROP TABLE source, source2; DROP FUNCTION merge_trigfunc(); --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace040-waitfuncs-v2.patch" Author: Noah Misch Commit: Noah Misch Create waitfuncs.c for pg_isolation_test_session_is_blocked(). The next commit makes the function inspect an additional non-lock contention source, so it no longer fits in lockfuncs.c. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile index 610ccf2..edb09d4 100644 --- a/src/backend/utils/adt/Makefile +++ b/src/backend/utils/adt/Makefile @@ -116,6 +116,7 @@ OBJS = \ varchar.o \ varlena.o \ version.o \ + waitfuncs.o \ windowfuncs.o \ xid.o \ xid8funcs.o \ diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c index 13009cc..e790f85 100644 --- a/src/backend/utils/adt/lockfuncs.c +++ b/src/backend/utils/adt/lockfuncs.c @@ -13,7 +13,6 @@ #include "postgres.h" #include "access/htup_details.h" -#include "catalog/pg_type.h" #include "funcapi.h" #include "miscadmin.h" #include "storage/predicate_internals.h" @@ -602,84 +601,6 @@ pg_safe_snapshot_blocking_pids(PG_FUNCTION_ARGS) /* - * pg_isolation_test_session_is_blocked - support function for isolationtester - * - * 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. - * - * This is an undocumented function intended for use by the isolation tester, - * and may change in future releases as required for testing purposes. - */ -Datum -pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS) -{ - int blocked_pid = PG_GETARG_INT32(0); - ArrayType *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1); - ArrayType *blocking_pids_a; - int32 *interesting_pids; - int32 *blocking_pids; - int num_interesting_pids; - int num_blocking_pids; - int dummy; - int i, - j; - - /* Validate the passed-in array */ - Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID); - if (array_contains_nulls(interesting_pids_a)) - elog(ERROR, "array must not contain nulls"); - interesting_pids = (int32 *) ARR_DATA_PTR(interesting_pids_a); - num_interesting_pids = ArrayGetNItems(ARR_NDIM(interesting_pids_a), - ARR_DIMS(interesting_pids_a)); - - /* - * Get the PIDs of all sessions blocking the given session's attempt to - * acquire heavyweight locks. - */ - blocking_pids_a = - DatumGetArrayTypeP(DirectFunctionCall1(pg_blocking_pids, blocked_pid)); - - Assert(ARR_ELEMTYPE(blocking_pids_a) == INT4OID); - Assert(!array_contains_nulls(blocking_pids_a)); - blocking_pids = (int32 *) ARR_DATA_PTR(blocking_pids_a); - num_blocking_pids = ArrayGetNItems(ARR_NDIM(blocking_pids_a), - ARR_DIMS(blocking_pids_a)); - - /* - * Check if any of these are in the list of interesting PIDs, that being - * the sessions that the isolation tester is running. We don't use - * "arrayoverlaps" here, because it would lead to cache lookups and one of - * our goals is to run quickly with debug_discard_caches > 0. We expect - * blocking_pids to be usually empty and otherwise a very small number in - * isolation tester cases, so make that the outer loop of a naive search - * for a match. - */ - for (i = 0; i < num_blocking_pids; i++) - for (j = 0; j < num_interesting_pids; j++) - { - if (blocking_pids[i] == interesting_pids[j]) - PG_RETURN_BOOL(true); - } - - /* - * Check if blocked_pid is waiting for a safe snapshot. We could in - * theory check the resulting array of blocker PIDs against the - * interesting PIDs list, but since there is no danger of autovacuum - * blocking GetSafeSnapshot there seems to be no point in expending cycles - * on allocating a buffer and searching for overlap; so it's presently - * sufficient for the isolation tester's purposes to use a single element - * buffer and check if the number of safe snapshot blockers is non-zero. - */ - if (GetSafeSnapshotBlockingPids(blocked_pid, &dummy, 1) > 0) - PG_RETURN_BOOL(true); - - PG_RETURN_BOOL(false); -} - - -/* * Functions for manipulating advisory locks * * We make use of the locktag fields as follows: diff --git a/src/backend/utils/adt/meson.build b/src/backend/utils/adt/meson.build index 48dbcf5..8c6fc80 100644 --- a/src/backend/utils/adt/meson.build +++ b/src/backend/utils/adt/meson.build @@ -103,6 +103,7 @@ backend_sources += files( 'varchar.c', 'varlena.c', 'version.c', + 'waitfuncs.c', 'windowfuncs.c', 'xid.c', 'xid8funcs.c', diff --git a/src/backend/utils/adt/waitfuncs.c b/src/backend/utils/adt/waitfuncs.c new file mode 100644 index 0000000..d9c92c3 --- /dev/null +++ b/src/backend/utils/adt/waitfuncs.c @@ -0,0 +1,96 @@ +/*------------------------------------------------------------------------- + * + * waitfuncs.c + * Functions for SQL access to syntheses of multiple contention types. + * + * Copyright (c) 2002-2024, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/backend/utils/adt/waitfuncs.c + * + *------------------------------------------------------------------------- + */ +#include "postgres.h" + +#include "catalog/pg_type.h" +#include "storage/predicate_internals.h" +#include "utils/array.h" +#include "utils/builtins.h" + + +/* + * pg_isolation_test_session_is_blocked - support function for isolationtester + * + * 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. + * + * This is an undocumented function intended for use by the isolation tester, + * and may change in future releases as required for testing purposes. + */ +Datum +pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS) +{ + int blocked_pid = PG_GETARG_INT32(0); + ArrayType *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1); + ArrayType *blocking_pids_a; + int32 *interesting_pids; + int32 *blocking_pids; + int num_interesting_pids; + int num_blocking_pids; + int dummy; + int i, + j; + + /* Validate the passed-in array */ + Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID); + if (array_contains_nulls(interesting_pids_a)) + elog(ERROR, "array must not contain nulls"); + interesting_pids = (int32 *) ARR_DATA_PTR(interesting_pids_a); + num_interesting_pids = ArrayGetNItems(ARR_NDIM(interesting_pids_a), + ARR_DIMS(interesting_pids_a)); + + /* + * Get the PIDs of all sessions blocking the given session's attempt to + * acquire heavyweight locks. + */ + blocking_pids_a = + DatumGetArrayTypeP(DirectFunctionCall1(pg_blocking_pids, blocked_pid)); + + Assert(ARR_ELEMTYPE(blocking_pids_a) == INT4OID); + Assert(!array_contains_nulls(blocking_pids_a)); + blocking_pids = (int32 *) ARR_DATA_PTR(blocking_pids_a); + num_blocking_pids = ArrayGetNItems(ARR_NDIM(blocking_pids_a), + ARR_DIMS(blocking_pids_a)); + + /* + * Check if any of these are in the list of interesting PIDs, that being + * the sessions that the isolation tester is running. We don't use + * "arrayoverlaps" here, because it would lead to cache lookups and one of + * our goals is to run quickly with debug_discard_caches > 0. We expect + * blocking_pids to be usually empty and otherwise a very small number in + * isolation tester cases, so make that the outer loop of a naive search + * for a match. + */ + for (i = 0; i < num_blocking_pids; i++) + for (j = 0; j < num_interesting_pids; j++) + { + if (blocking_pids[i] == interesting_pids[j]) + PG_RETURN_BOOL(true); + } + + /* + * Check if blocked_pid is waiting for a safe snapshot. We could in + * theory check the resulting array of blocker PIDs against the + * interesting PIDs list, but since there is no danger of autovacuum + * blocking GetSafeSnapshot there seems to be no point in expending cycles + * on allocating a buffer and searching for overlap; so it's presently + * sufficient for the isolation tester's purposes to use a single element + * buffer and check if the number of safe snapshot blockers is non-zero. + */ + if (GetSafeSnapshotBlockingPids(blocked_pid, &dummy, 1) > 0) + PG_RETURN_BOOL(true); + + PG_RETURN_BOOL(false); +} --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace050-tests-inj-v2.patch" Author: Noah Misch Commit: Noah Misch Add an injection_points isolation test suite. Make the isolation harness recognize injection_points wait events as a type of blocked state. To simplify that, change that wait event naming scheme to INJECTION_POINT(name). Add an initial test for an extant inplace-update bug. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4be0dee..4eda445 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -63,6 +63,7 @@ #include "storage/procarray.h" #include "storage/standby.h" #include "utils/datum.h" +#include "utils/injection_point.h" #include "utils/inval.h" #include "utils/relcache.h" #include "utils/snapmgr.h" @@ -6077,6 +6078,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple) (errcode(ERRCODE_INVALID_TRANSACTION_STATE), errmsg("cannot update tuples during a parallel operation"))); + INJECTION_POINT("inplace-before-pin"); buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self))); LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); page = (Page) BufferGetPage(buffer); diff --git a/src/backend/utils/adt/waitfuncs.c b/src/backend/utils/adt/waitfuncs.c index d9c92c3..b524a8a 100644 --- a/src/backend/utils/adt/waitfuncs.c +++ b/src/backend/utils/adt/waitfuncs.c @@ -14,8 +14,13 @@ #include "catalog/pg_type.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" + +#define UINT32_ACCESS_ONCE(var) ((uint32)(*((volatile uint32 *)&(var)))) /* @@ -23,8 +28,9 @@ * * 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. @@ -34,6 +40,8 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS) { 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; @@ -43,6 +51,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..2ffd2f7 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 + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/injection_points/expected/inplace.out b/src/test/modules/injection_points/expected/inplace.out new file mode 100644 index 0000000..123f45a --- /dev/null +++ b/src/test/modules/injection_points/expected/inplace.out @@ -0,0 +1,43 @@ +Parsed test spec with 3 sessions + +starting permutation: vac1 grant2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +ERROR: could not create unique index "pg_class_oid_index" diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 5c44625..4193061 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -200,6 +200,7 @@ injection_notice(const char *name, const void *private_data) void injection_wait(const char *name, const void *private_data) { + char event_name[NAMEDATALEN]; uint32 old_wait_counts = 0; int index = -1; uint32 injection_wait_event = 0; @@ -212,11 +213,11 @@ injection_wait(const char *name, const void *private_data) 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. diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 8e1b5b4..3c23c14 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -37,4 +37,9 @@ tests += { # The injection points are cluster-wide, so disable installcheck 'runningcheck': false, }, + 'isolation': { + 'specs': [ + 'inplace', + ], + }, } diff --git a/src/test/modules/injection_points/specs/inplace.spec b/src/test/modules/injection_points/specs/inplace.spec new file mode 100644 index 0000000..e957713 --- /dev/null +++ b/src/test/modules/injection_points/specs/inplace.spec @@ -0,0 +1,83 @@ +# Test race conditions involving: +# - s1: VACUUM inplace-updating a pg_class row +# - s2: GRANT/REVOKE making pg_class rows dead +# - s3: "VACUUM pg_class" making dead rows LP_UNUSED; DDL reusing them + +# Need GRANT to make a non-HOT update. Otherwise, "VACUUM pg_class" would +# leave an LP_REDIRECT that persists. To get non-HOT, make rels so the +# pg_class row for vactest.orig50 is on a filled page (assuming BLCKSZ=8192). +# Just to save on filesystem syscalls, use relkind=c for every other rel. +setup +{ + CREATE EXTENSION injection_points; + CREATE SCHEMA vactest; + CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void + LANGUAGE plpgsql SET search_path = vactest AS $$ + DECLARE + tname text; + BEGIN + FOR i in $2 .. $3 LOOP + tname := $1 || i; + EXECUTE FORMAT('CREATE TYPE ' || tname || ' AS ()'); + RAISE DEBUG '% at %', tname, ctid + FROM pg_class WHERE oid = tname::regclass; + END LOOP; + END + $$; +} +setup { VACUUM FULL pg_class; -- reduce free space } +setup +{ + SELECT vactest.mkrels('orig', 1, 49); + CREATE TABLE vactest.orig50 (); + SELECT vactest.mkrels('orig', 51, 100); +} + +# XXX DROP causes an assertion failure; adopt DROP once fixed +teardown +{ + --DROP SCHEMA vactest CASCADE; + DO $$BEGIN EXECUTE 'ALTER SCHEMA vactest RENAME TO schema' || oid FROM pg_namespace where nspname = 'vactest'; END$$; + DROP EXTENSION injection_points; +} + +# Wait during inplace update, in a VACUUM of vactest.orig50. +session s1 +setup { + SELECT injection_points_set_local(); + SELECT injection_points_attach('inplace-before-pin', 'wait'); +} +step vac1 { VACUUM vactest.orig50; -- wait during inplace update } +# One bug scenario leaves two live pg_class tuples for vactest.orig50 and zero +# live tuples for one of the "intruder" rels. REINDEX observes the duplicate. +step read1 { + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; +} + + +# Transactional updates of the tuple vac1 is waiting to inplace-update. +session s2 +step grant2 { GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; } + + +# Non-blocking actions. +session s3 +step vac3 { VACUUM pg_class; } +# Reuse the lp that vac1 is waiting to change. I've observed reuse at the 1st +# or 18th CREATE, so create excess. +step mkrels3 { + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); +} + + +# XXX extant bug +permutation + vac1(mkrels3) # reads pg_class tuple T0 for vactest.orig50, xmax invalid + grant2 # T0 becomes eligible for pruning, T1 is successor + vac3 # T0 becomes LP_UNUSED + mkrels3 # T0 reused; vac1 wakes and overwrites the reused T0 + read1 diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.pl index a792610..18c800c 100644 --- a/src/test/modules/test_misc/t/005_timeouts.pl +++ b/src/test/modules/test_misc/t/005_timeouts.pl @@ -50,7 +50,8 @@ $psql_session->query_until( # Wait until the backend enters the timeout injection point. Will get an error # here if anything goes wrong. -$node->wait_for_event('client backend', 'transaction-timeout'); +$node->wait_for_event('client backend', + 'INJECTION_POINT(transaction-timeout)'); my $log_offset = -s $node->logfile; @@ -86,7 +87,7 @@ $psql_session->query_until( # Wait until the backend enters the timeout injection point. $node->wait_for_event('client backend', - 'idle-in-transaction-session-timeout'); + 'INJECTION_POINT(idle-in-transaction-session-timeout)'); $log_offset = -s $node->logfile; @@ -116,7 +117,8 @@ $psql_session->query_until( )); # Wait until the backend enters the timeout injection point. -$node->wait_for_event('client backend', 'idle-session-timeout'); +$node->wait_for_event('client backend', + 'INJECTION_POINT(idle-session-timeout)'); $log_offset = -s $node->logfile; diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl index 7c30731..538facb 100644 --- a/src/test/recovery/t/041_checkpoint_at_promote.pl +++ b/src/test/recovery/t/041_checkpoint_at_promote.pl @@ -78,7 +78,8 @@ $node_primary->wait_for_replay_catchup($node_standby); # Wait until the checkpointer is in the middle of the restart point # processing. -$node_standby->wait_for_event('checkpointer', 'create-restart-point'); +$node_standby->wait_for_event('checkpointer', + 'INJECTION_POINT(create-restart-point)'); # Check the logs that the restart point has started on standby. This is # optional, but let's be sure. --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace060-nodeModifyTable-comments-v2.patch" Author: Noah Misch Commit: Noah Misch Expand comments and add an assertion in nodeModifyTable.c. Most comments concern RELKIND_VIEW. One addresses the ExecUpdate() "tupleid" parameter. A later commit will rely on these facts, but they hold already. Back-patch to v12 (all supported versions), the plan for that commit. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index cee60d3..a2442b7 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -24,6 +24,14 @@ * values plus row-locating info for UPDATE and MERGE cases, or just the * row-locating info for DELETE cases. * + * The relation to modify can be an ordinary table, a view having an + * INSTEAD OF trigger, or a foreign table. Earlier processing already + * pointed ModifyTable to the underlying relations of any automatically + * updatable view not using an INSTEAD OF trigger, so code here can + * assume it won't have one as a modification target. This node does + * process ri_WithCheckOptions, which may have expressions from those + * automatically updatable views. + * * MERGE runs a join between the source relation and the target table. * If any WHEN NOT MATCHED [BY TARGET] clauses are present, then the join * is an outer join that might output tuples without a matching target @@ -1398,18 +1406,18 @@ ExecDeleteEpilogue(ModifyTableContext *context, ResultRelInfo *resultRelInfo, * DELETE is like UPDATE, except that we delete the tuple and no * index modifications are needed. * - * When deleting from a table, tupleid identifies the tuple to - * delete and oldtuple is NULL. When deleting from a view, - * oldtuple is passed to the INSTEAD OF triggers and identifies - * what to delete, and tupleid is invalid. When deleting from a - * foreign table, tupleid is invalid; the FDW has to figure out - * which row to delete using data from the planSlot. oldtuple is - * passed to foreign table triggers; it is NULL when the foreign - * table has no relevant triggers. We use tupleDeleted to indicate - * whether the tuple is actually deleted, callers can use it to - * decide whether to continue the operation. When this DELETE is a - * part of an UPDATE of partition-key, then the slot returned by - * EvalPlanQual() is passed back using output parameter epqreturnslot. + * When deleting from a table, tupleid identifies the tuple to delete and + * oldtuple is NULL. When deleting through a view INSTEAD OF trigger, + * oldtuple is passed to the triggers and identifies what to delete, and + * tupleid is invalid. When deleting from a foreign table, tupleid is + * invalid; the FDW has to figure out which row to delete using data from + * the planSlot. oldtuple is passed to foreign table triggers; it is + * NULL when the foreign table has no relevant triggers. We use + * tupleDeleted to indicate whether the tuple is actually deleted, + * callers can use it to decide whether to continue the operation. When + * this DELETE is a part of an UPDATE of partition-key, then the slot + * returned by EvalPlanQual() is passed back using output parameter + * epqreturnslot. * * Returns RETURNING result if any, otherwise NULL. * ---------------------------------------------------------------- @@ -2238,21 +2246,22 @@ ExecCrossPartitionUpdateForeignKey(ModifyTableContext *context, * is, we don't want to get stuck in an infinite loop * which corrupts your database.. * - * When updating a table, tupleid identifies the tuple to - * update and oldtuple is NULL. When updating a view, oldtuple - * is passed to the INSTEAD OF triggers and identifies what to - * update, and tupleid is invalid. When updating a foreign table, - * tupleid is invalid; the FDW has to figure out which row to - * update using data from the planSlot. oldtuple is passed to - * foreign table triggers; it is NULL when the foreign table has - * no relevant triggers. + * When updating a table, tupleid identifies the tuple to update and + * oldtuple is NULL. When updating through a view INSTEAD OF trigger, + * oldtuple is passed to the triggers and identifies what to update, and + * tupleid is invalid. When updating a foreign table, tupleid is + * invalid; the FDW has to figure out which row to update using data from + * the planSlot. oldtuple is passed to foreign table triggers; it is + * NULL when the foreign table has no relevant triggers. * * slot contains the new tuple value to be stored. * planSlot is the output of the ModifyTable's subplan; we use it * to access values from other input tables (for RETURNING), * row-ID junk columns, etc. * - * Returns RETURNING result if any, otherwise NULL. + * Returns RETURNING result if any, otherwise NULL. On exit, if tupleid + * had identified the tuple to update, it will identify the tuple + * actually updated after EvalPlanQual. * ---------------------------------------------------------------- */ static TupleTableSlot * @@ -2717,10 +2726,10 @@ ExecMerge(ModifyTableContext *context, ResultRelInfo *resultRelInfo, /*----- * If we are dealing with a WHEN MATCHED case, tupleid or oldtuple is - * valid, depending on whether the result relation is a table or a view. - * We execute the first action for which the additional WHEN MATCHED AND - * quals pass. If an action without quals is found, that action is - * executed. + * valid, depending on whether the result relation is a table or a view + * having an INSTEAD OF trigger. We execute the first action for which + * the additional WHEN MATCHED AND quals pass. If an action without quals + * is found, that action is executed. * * Similarly, in the WHEN NOT MATCHED BY SOURCE case, tupleid or oldtuple * is valid, and we look at the given WHEN NOT MATCHED BY SOURCE actions @@ -2811,8 +2820,8 @@ ExecMerge(ModifyTableContext *context, ResultRelInfo *resultRelInfo, * Check and execute the first qualifying MATCHED or NOT MATCHED BY SOURCE * action, depending on whether the join quals are satisfied. If the target * relation is a table, the current target tuple is identified by tupleid. - * Otherwise, if the target relation is a view, oldtuple is the current target - * tuple from the view. + * Otherwise, if the target relation is a view having an INSTEAD OF trigger, + * oldtuple is the current target tuple from the view. * * We start from the first WHEN MATCHED or WHEN NOT MATCHED BY SOURCE action * and check if the WHEN quals pass, if any. If the WHEN quals for the first @@ -2878,8 +2887,11 @@ ExecMergeMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo, */ Assert(tupleid != NULL || oldtuple != NULL); if (oldtuple != NULL) + { + Assert(resultRelInfo->ri_TrigDesc); ExecForceStoreHeapTuple(oldtuple, resultRelInfo->ri_oldTupleSlot, false); + } else if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc, tupleid, SnapshotAny, @@ -3992,8 +4004,8 @@ ExecModifyTable(PlanState *pstate) * know enough here to set t_tableOid. Quite separately from * this, the FDW may fetch its own junk attrs to identify the row. * - * Other relevant relkinds, currently limited to views, always - * have a wholerow attribute. + * Other relevant relkinds, currently limited to views having + * INSTEAD OF triggers, always have a wholerow attribute. */ else if (AttributeNumberIsValid(resultRelInfo->ri_RowIdAttNo)) { --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace065-lock-SequenceChangePersistence-v2.patch" Author: Noah Misch Commit: Noah Misch Lock owned sequences during ALTER TABLE SET { LOGGED | UNLOGGED }. These commands already make the persistence of owned sequences follow owned table persistence changes. They didn't lock those sequences. They lost the effect of nextval() calls that other sessions make after the ALTER TABLE command, before the ALTER TABLE transaction commits. Fix by acquiring the same lock that ALTER SEQUENCE SET { LOGGED | UNLOGGED } acquires. This might cause more deadlocks. Back-patch to v15, where commit 344d62fb9a978a72cf8347f0369b9ee643fd0b31 introduced unlogged sequences. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index 4610356..d020867 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -545,6 +545,13 @@ SequenceChangePersistence(Oid relid, char newrelpersistence) Buffer buf; HeapTupleData seqdatatuple; + /* + * ALTER SEQUENCE acquires this lock earlier. If we're processing an + * owned sequence for ALTER TABLE, lock now. Without the lock, we'd + * discard increments from nextval() calls (in other sessions) between + * this function's buffer unlock and this transaction's commit. + */ + LockRelationOid(relid, AccessExclusiveLock); init_sequence(relid, &elm, &seqrel); /* check the comment above nextval_internal()'s equivalent call. */ --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace071-lock-SetRelationHasSubclass-v2.patch" Author: Noah Misch Commit: Noah Misch Lock before setting relhassubclass on RELKIND_PARTITIONED_INDEX. Commit 5b562644fec696977df4a82790064e8287927891 added a comment that SetRelationHasSubclass() callers must hold this lock. When commit 17f206fbc824d2b4b14480199ca9ff7dea417eda extended use of this column to partitioned indexes, it didn't take the lock. As the latter commit message mentioned, we currently never reset a partitioned index to relhassubclass=f. That largely avoids harm from the lock omission. The cause for fixing this now is to unblock introducing a rule about locks required to heap_update() a pg_class row. This might cause more deadlocks. It gives minor user-visible benefits: - If an ALTER INDEX SET TABLESPACE runs concurrently with ALTER TABLE ATTACH PARTITION or CREATE PARTITION OF, one transaction blocks instead of failing with "tuple concurrently updated". (Many cases of DDL concurrency still fail that way.) - Match ALTER INDEX ATTACH PARTITION in choosing to lock the index. While not user-visible today, we'll need this if we ever make something set the flag to false for a partitioned index, like ANALYZE does today for tables. Back-patch to v12 (all supported versions), the plan for the commit relying on the new rule. In back branches, add LockOrStrongerHeldByMe() instead of adding a LockHeldByMe() parameter. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 5a8568c..5c48e57 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1058,6 +1058,7 @@ index_create(Relation heapRelation, if (OidIsValid(parentIndexRelid)) { StoreSingleInheritance(indexRelationId, parentIndexRelid, 1); + LockRelationOid(parentIndexRelid, ShareUpdateExclusiveLock); SetRelationHasSubclass(parentIndexRelid, true); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index d9016ef..716e0e8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4475,7 +4475,10 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid) /* set relhassubclass if an index partition has been added to the parent */ if (OidIsValid(parentOid)) + { + LockRelationOid(parentOid, ShareUpdateExclusiveLock); SetRelationHasSubclass(parentOid, true); + } /* set relispartition correctly on the partition */ update_relispartition(partRelid, OidIsValid(parentOid)); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index de0d911..7b66c57 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3581,8 +3581,15 @@ findAttrByName(const char *attributeName, const List *columns) * SetRelationHasSubclass * Set the value of the relation's relhassubclass field in pg_class. * - * NOTE: caller must be holding an appropriate lock on the relation. - * ShareUpdateExclusiveLock is sufficient. + * It's always safe to set this field to true, because all SQL commands are + * ready to see true and then find no children. On the other hand, commands + * generally assume zero children if this is false. + * + * Caller must hold any self-exclusive lock until end of transaction. If the + * new value is false, caller must have acquired that lock before reading the + * evidence that justified the false value. That way, it properly waits if + * another backend is simultaneously concluding no need to change the tuple + * (new and old values are true). * * NOTE: an important side-effect of this operation is that an SI invalidation * message is sent out to all backends --- including me --- causing plans @@ -3597,6 +3604,11 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass) HeapTuple tuple; Form_pg_class classtuple; + Assert(CheckRelationOidLockedByMe(relationId, + ShareUpdateExclusiveLock, false) || + CheckRelationOidLockedByMe(relationId, + ShareRowExclusiveLock, true)); + /* * Fetch a modifiable copy of the tuple, modify it, update pg_class. */ diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index fe3cda2..094522a 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -335,32 +335,22 @@ CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger) relation->rd_lockInfo.lockRelId.dbId, relation->rd_lockInfo.lockRelId.relId); - if (LockHeldByMe(&tag, lockmode)) - return true; + return LockHeldByMe(&tag, lockmode, orstronger); +} - if (orstronger) - { - LOCKMODE slockmode; +/* + * CheckRelationOidLockedByMe + * + * Like the above, but takes an OID as argument. + */ +bool +CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, bool orstronger) +{ + LOCKTAG tag; - for (slockmode = lockmode + 1; - slockmode <= MaxLockMode; - slockmode++) - { - if (LockHeldByMe(&tag, slockmode)) - { -#ifdef NOT_USED - /* Sometimes this might be useful for debugging purposes */ - elog(WARNING, "lock mode %s substituted for %s on relation %s", - GetLockmodeName(tag.locktag_lockmethodid, slockmode), - GetLockmodeName(tag.locktag_lockmethodid, lockmode), - RelationGetRelationName(relation)); -#endif - return true; - } - } - } + SetLocktagRelationOid(&tag, relid); - return false; + return LockHeldByMe(&tag, lockmode, orstronger); } /* diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 5154353..1bf3e6d 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -578,11 +578,17 @@ DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2) } /* - * LockHeldByMe -- test whether lock 'locktag' is held with mode 'lockmode' - * by the current transaction + * LockHeldByMe -- test whether lock 'locktag' is held by the current + * transaction + * + * Returns true if current transaction holds a lock on 'tag' of mode + * 'lockmode'. If 'orstronger' is true, a stronger lockmode is also OK. + * ("Stronger" is defined as "numerically higher", which is a bit + * semantically dubious but is OK for the purposes we use this for.) */ bool -LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode) +LockHeldByMe(const LOCKTAG *locktag, + LOCKMODE lockmode, bool orstronger) { LOCALLOCKTAG localtag; LOCALLOCK *locallock; @@ -598,7 +604,23 @@ LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode) &localtag, HASH_FIND, NULL); - return (locallock && locallock->nLocks > 0); + if (locallock && locallock->nLocks > 0) + return true; + + if (orstronger) + { + LOCKMODE slockmode; + + for (slockmode = lockmode + 1; + slockmode <= MaxLockMode; + slockmode++) + { + if (LockHeldByMe(locktag, slockmode, false)) + return true; + } + } + + return false; } #ifdef USE_ASSERT_CHECKING diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h index 22b7856..ce15125 100644 --- a/src/include/storage/lmgr.h +++ b/src/include/storage/lmgr.h @@ -48,6 +48,8 @@ extern bool ConditionalLockRelation(Relation relation, LOCKMODE lockmode); extern void UnlockRelation(Relation relation, LOCKMODE lockmode); extern bool CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger); +extern bool CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, + bool orstronger); extern bool LockHasWaitersRelation(Relation relation, LOCKMODE lockmode); extern void LockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode); diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h index 0017d4b..cc1f6e7 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -567,7 +567,8 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks); extern void LockReleaseSession(LOCKMETHODID lockmethodid); extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks); extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks); -extern bool LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode); +extern bool LockHeldByMe(const LOCKTAG *locktag, + LOCKMODE lockmode, bool orstronger); #ifdef USE_ASSERT_CHECKING extern HTAB *GetLockMethodLocalHash(void); #endif --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace075-lock-heap_create-v2.patch" Author: Noah Misch Commit: Noah Misch AccessExclusiveLock new relations just after assigning the OID. This has no user-visible, important consequences, since other sessions' catalog scans can't find the relation until we commit. However, this unblocks introducing a rule about locks required to heap_update() a pg_class row. CREATE TABLE has been acquiring this lock eventually, but it can heap_update() pg_class.relchecks earlier. create_toast_table() has been acquiring only ShareLock. Back-patch to v12 (all supported versions), the plan for the commit relying on the new rule. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 922ba79..fbad1d2 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -1250,6 +1250,13 @@ heap_create_with_catalog(const char *relname, } /* + * Other sessions' catalog scans can't find this until we commit. Hence, + * it doesn't hurt to hold AccessExclusiveLock. Do it here so callers + * can't accidentally vary in their lock mode or acquisition timing. + */ + LockRelationOid(relid, AccessExclusiveLock); + + /* * Determine the relation's initial permissions. */ if (use_user_acl) --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace080-catcache-detoast-inplace-stale-v2.patch" Author: Noah Misch Commit: Noah Misch Cope with inplace update making catcache stale during TOAST fetch. This extends ad98fb14226ae6456fbaed7990ee7591cbe5efd2 to invals of inplace updates. Trouble requires an inplace update of a catalog having a TOAST table, so only pg_database was at risk. (The other catalog on which core code performs inplace updates, pg_class, has no TOAST table.) Trouble would require something like the inplace-inval.spec test. Consider GRANT ... ON DATABASE fetching a stale row from cache and discarding a datfrozenxid update that vac_truncate_clog() has already relied upon. Back-patch to v12 (all supported versions). Reviewed by FIXME. Discussion: https://postgr.es/m/20240114201411.d0@rfd.leadboat.com Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 3217008..6c39434 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -136,6 +136,27 @@ IsCatalogRelationOid(Oid relid) } /* + * IsInplaceUpdateRelation + * True iff core code performs inplace updates on the relation. + */ +bool +IsInplaceUpdateRelation(Relation relation) +{ + return IsInplaceUpdateOid(RelationGetRelid(relation)); +} + +/* + * IsInplaceUpdateOid + * Like the above, but takes an OID as argument. + */ +bool +IsInplaceUpdateOid(Oid relid) +{ + return (relid == RelationRelationId || + relid == DatabaseRelationId); +} + +/* * IsToastRelation * True iff relation is a TOAST support relation (or index). * diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c index 569f51c..98aa527 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -2008,6 +2008,23 @@ ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner) /* + * equalTuple + * Are these tuples memcmp()-equal? + */ +static bool +equalTuple(HeapTuple a, HeapTuple b) +{ + uint32 alen; + uint32 blen; + + alen = a->t_len; + blen = b->t_len; + return (alen == blen && + memcmp((char *) a->t_data, + (char *) b->t_data, blen) == 0); +} + +/* * CatalogCacheCreateEntry * Create a new CatCTup entry, copying the given HeapTuple and other * supplied data into it. The new entry initially has refcount 0. @@ -2057,14 +2074,34 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, SysScanDesc scandesc, */ if (HeapTupleHasExternal(ntp)) { + bool need_cmp = IsInplaceUpdateOid(cache->cc_reloid); + HeapTuple before = NULL; + bool matches = true; + + if (need_cmp) + before = heap_copytuple(ntp); dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc); /* * The tuple could become stale while we are doing toast table - * access (since AcceptInvalidationMessages can run then), so we - * must recheck its visibility afterwards. + * access (since AcceptInvalidationMessages can run then). + * equalTuple() detects staleness from inplace updates, while + * systable_recheck_tuple() detects staleness from normal updates. + * + * While this equalTuple() follows the usual rule of reading with + * a pin and no buffer lock, it warrants suspicion since an + * inplace update could appear at any moment. It's safe because + * the inplace update sends an invalidation that can't reorder + * before the inplace heap change. If the heap change reaches + * this process just after equalTuple() looks, we've not missed + * its inval. */ - if (!systable_recheck_tuple(scandesc, ntp)) + if (need_cmp) + { + matches = equalTuple(before, ntp); + heap_freetuple(before); + } + if (!matches || !systable_recheck_tuple(scandesc, ntp)) { heap_freetuple(dtp); return NULL; diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h index 1fd326e..a8dd304 100644 --- a/src/include/catalog/catalog.h +++ b/src/include/catalog/catalog.h @@ -21,11 +21,13 @@ extern bool IsSystemRelation(Relation relation); extern bool IsToastRelation(Relation relation); extern bool IsCatalogRelation(Relation relation); +extern bool IsInplaceUpdateRelation(Relation relation); extern bool IsSystemClass(Oid relid, Form_pg_class reltuple); extern bool IsToastClass(Form_pg_class reltuple); extern bool IsCatalogRelationOid(Oid relid); +extern bool IsInplaceUpdateOid(Oid relid); extern bool IsCatalogNamespace(Oid namespaceId); extern bool IsToastNamespace(Oid namespaceId); --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace090-LOCKTAG_TUPLE-eoxact-v2.patch" Author: Noah Misch Commit: Noah Misch Warn if LOCKTAG_TUPLE is held at commit, under debug_assertions. The current use always releases this locktag. A planned use will continue that intent. It will involve more areas of code, making unlock omissions easier. Warn under debug_assertions, like we do for various resource leaks. Back-patch to v12 (all supported versions), the plan for the commit of the new use. Reviewed by FIXME. Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 1bf3e6d..60b746d 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -2256,6 +2256,11 @@ LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks) locallock->numLockOwners = 0; } +#ifdef USE_ASSERT_CHECKING + if (LOCALLOCK_LOCKTAG(*locallock) == LOCKTAG_TUPLE && !allLocks) + elog(WARNING, "tuple lock held at commit"); +#endif + /* * If the lock or proclock pointers are NULL, this lock was taken via * the relation fast-path (and is not known to have been transferred). --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace110-successors-v2.patch" Author: Noah Misch Commit: Noah Misch Fix data loss at inplace update after heap_update(). As previously-added tests demonstrated, heap_inplace_update() could instead update an unrelated tuple of the same catalog. It could lose the update. Losing relhasindex=t was a source of index corruption. Inplace-updating commands like VACUUM will now wait for heap_update() commands like GRANT TABLE and GRANT DATABASE. That isn't ideal, but a long-running GRANT already hurts VACUUM progress more just by keeping an XID running. The VACUUM will behave like a DELETE or UPDATE waiting for the uncommitted change. For implementation details, start at the heap_inplace_update_scan() header comment and README.tuplock. Back-patch to v12 (all supported versions). In back branches, retain a deprecated heap_inplace_update(), for extensions. Reviewed by FIXME. Discussion: https://postgr.es/m/CAMp+ueZQz3yDk7qg42hk6-9gxniYbp-=bG2mgqecErqR5gGGOA@mail.gmail.com diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock index 6441e8b..dbfa2b7 100644 --- a/src/backend/access/heap/README.tuplock +++ b/src/backend/access/heap/README.tuplock @@ -153,3 +153,56 @@ The following infomask bits are applicable: We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit is set. + +Locking to write inplace-updated tables +--------------------------------------- + +[This is the plan, but LOCKTAG_TUPLE acquisition is not yet here.] + +If IsInplaceUpdateRelation() returns true for a table, the table is a system +catalog that receives heap_inplace_update_scan() calls. Preparing a +heap_update() of these tables follows additional locking rules, to ensure we +don't lose the effects of an inplace update. In particular, consider a moment +when a backend has fetched the old tuple to modify, not yet having called +heap_update(). Another backend's inplace update starting then can't conclude +until the heap_update() places its new tuple in a buffer. We enforce that +using locktags as follows. While DDL code is the main audience, the executor +follows these rules to make e.g. "MERGE INTO pg_class" safer. Locking rules +are per-catalog: + + pg_class heap_inplace_update_scan() callers: before the call, acquire + LOCKTAG_RELATION in mode ShareLock (CREATE INDEX), ShareUpdateExclusiveLock + (VACUUM), or a mode with strictly more conflicts. If the update targets a + row of RELKIND_INDEX (but not RELKIND_PARTITIONED_INDEX), that lock must be + on the table. Locking the index rel is optional. (This allows VACUUM to + overwrite per-index pg_class while holding a lock on the table alone.) We + could allow weaker locks, in which case the next paragraph would simply call + for stronger locks for its class of commands. heap_inplace_update_scan() + acquires and releases LOCKTAG_TUPLE in InplaceUpdateTupleLock, an alias for + ExclusiveLock, on each tuple it overwrites. + + pg_class heap_update() callers: before copying the tuple to modify, take a + lock that conflicts with at least one of those from the preceding paragraph. + SearchSysCacheLocked1() is one convenient way to acquire LOCKTAG_TUPLE. + After heap_update(), release any LOCKTAG_TUPLE. Most of these callers opt + to acquire just the LOCKTAG_RELATION. + + pg_database: before copying the tuple to modify, all updaters of pg_database + rows acquire LOCKTAG_TUPLE. (Few updaters acquire LOCKTAG_OBJECT on the + database OID, so it wasn't worth extending that as a second option.) + +Ideally, DDL might want to perform permissions checks before LockTuple(), as +we do with RangeVarGetRelidExtended() callbacks. We typically don't bother. +LOCKTAG_TUPLE acquirers release it after each row, so the potential +inconvenience is lower. + +Reading inplace-updated columns +------------------------------- + +Inplace updates create an exception to the rule that tuple data won't change +under a reader holding a pin. A reader of a heap_fetch() result tuple may +witness a torn read. Current inplace-updated fields are aligned and are no +wider than four bytes, and current readers don't need consistency across +fields. Hence, they get by with just fetching each field once. XXX such a +caller may also read a value that has not reached WAL; see +heap_inplace_update_finish(). diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 4eda445..f1d4fc0 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -76,6 +76,9 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf, Buffer newbuf, HeapTuple oldtup, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared); +#ifdef USE_ASSERT_CHECKING +static void check_inplace_rel_lock(HeapTuple oldtup); +#endif static Bitmapset *HeapDetermineColumnsInfo(Relation relation, Bitmapset *interesting_cols, Bitmapset *external_cols, @@ -97,6 +100,7 @@ static void compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask, static TM_Result heap_lock_updated_tuple(Relation rel, HeapTuple tuple, ItemPointer ctid, TransactionId xid, LockTupleMode mode); +static bool inplace_xmax_lock(SysScanDesc scan); static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask, uint16 *new_infomask2); static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax, @@ -4069,6 +4073,45 @@ l2: return TM_Ok; } +#ifdef USE_ASSERT_CHECKING +/* + * Confirm adequate relation lock held, per rules from README.tuplock section + * "Locking to write inplace-updated tables". + */ +static void +check_inplace_rel_lock(HeapTuple oldtup) +{ + Form_pg_class classForm = (Form_pg_class) GETSTRUCT(oldtup); + Oid relid = classForm->oid; + Oid dbid; + LOCKTAG tag; + + if (IsSharedRelation(relid)) + dbid = InvalidOid; + else + dbid = MyDatabaseId; + + if (classForm->relkind == RELKIND_INDEX) + { + Relation irel = index_open(relid, AccessShareLock); + + SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid); + index_close(irel, AccessShareLock); + } + else + SET_LOCKTAG_RELATION(tag, dbid, relid); + + if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock, true)) + elog(WARNING, + "missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)", + NameStr(classForm->relname), + relid, + classForm->relkind, + ItemPointerGetBlockNumber(&oldtup->t_self), + ItemPointerGetOffsetNumber(&oldtup->t_self)); +} +#endif + /* * Check if the specified attribute's values are the same. Subroutine for * HeapDetermineColumnsInfo. @@ -6038,34 +6081,45 @@ heap_abort_speculative(Relation relation, ItemPointer tid) } /* - * heap_inplace_update - update a tuple "in place" (ie, overwrite it) + * heap_inplace_update_scan - update a row "in place" (ie, overwrite it) * - * Overwriting violates both MVCC and transactional safety, so the uses - * of this function in Postgres are extremely limited. Nonetheless we - * find some places to use it. + * Overwriting violates both MVCC and transactional safety, so the uses of + * this function in Postgres are extremely limited. Nonetheless we find some + * places to use it. See README.tuplock section "Locking to write + * inplace-updated tables" and later sections for expectations of readers and + * writers of a table that gets inplace updates. Standard flow: * - * The tuple cannot change size, and therefore it's reasonable to assume - * that its null bitmap (if any) doesn't change either. So we just - * overwrite the data portion of the tuple without touching the null - * bitmap or any of the header fields. + * ... [any slow preparation not requiring oldtup] ... + * heap_inplace_update_scan([...], &tup, &inplace_state); + * if (!HeapTupleIsValid(tup)) + * elog(ERROR, [...]); + * ... [buffer is exclusive-locked; mutate "tup"] ... + * if (dirty) + * heap_inplace_update_finish(inplace_state, tup); + * else + * heap_inplace_update_cancel(inplace_state); * - * tuple is an in-memory tuple structure containing the data to be written - * over the target tuple. Also, tuple->t_self identifies the target tuple. + * Since this is intended for system catalogs and SERIALIZABLE doesn't cover + * DDL, this skips some predicate locks. * - * Note that the tuple updated here had better not come directly from the - * syscache if the relation has a toast relation as this tuple could - * include toast values that have been expanded, causing a failure here. + * The first several params duplicate the systable_beginscan() param list. + * "oldtupcopy" is an output parameter, assigned NULL if the key ceases to + * find a live tuple. (In PROC_IN_VACUUM, that is a low-probability transient + * condition.) If "oldtupcopy" gets non-NULL, you must pass output parameter + * "state" to heap_inplace_update_finish() or heap_inplace_update_cancel(). */ void -heap_inplace_update(Relation relation, HeapTuple tuple) +heap_inplace_update_scan(Relation relation, + Oid indexId, + bool indexOK, + Snapshot snapshot, + int nkeys, const ScanKeyData *key, + HeapTuple *oldtupcopy, void **state) { - Buffer buffer; - Page page; - OffsetNumber offnum; - ItemId lp = NULL; - HeapTupleHeader htup; - uint32 oldlen; - uint32 newlen; + ScanKey mutable_key = palloc(sizeof(ScanKeyData) * nkeys); + int retries = 0; + SysScanDesc scan; + HeapTuple oldtup; /* * For now, we don't allow parallel updates. Unlike a regular update, @@ -6078,21 +6132,70 @@ heap_inplace_update(Relation relation, HeapTuple tuple) (errcode(ERRCODE_INVALID_TRANSACTION_STATE), errmsg("cannot update tuples during a parallel operation"))); - INJECTION_POINT("inplace-before-pin"); - buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self))); - LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); - page = (Page) BufferGetPage(buffer); + /* + * Accept a snapshot argument, for symmetry, but this function advances + * its snapshot as needed to reach the tail of the updated tuple chain. + */ + Assert(snapshot == NULL); - offnum = ItemPointerGetOffsetNumber(&(tuple->t_self)); - if (PageGetMaxOffsetNumber(page) >= offnum) - lp = PageGetItemId(page, offnum); + Assert(IsInplaceUpdateRelation(relation) || !IsSystemRelation(relation)); - if (PageGetMaxOffsetNumber(page) < offnum || !ItemIdIsNormal(lp)) - elog(ERROR, "invalid lp"); + /* Loop for an exclusive-locked buffer of a non-updated tuple. */ + do + { + CHECK_FOR_INTERRUPTS(); - htup = (HeapTupleHeader) PageGetItem(page, lp); + /* + * Processes issuing heap_update (e.g. GRANT) at maximum speed could + * drive us to this error. A hostile table owner has stronger ways to + * damage their own table, so that's minor. + */ + if (retries++ > 10000) + elog(ERROR, "giving up after too many tries to overwrite row"); - oldlen = ItemIdGetLength(lp) - htup->t_hoff; + memcpy(mutable_key, key, sizeof(ScanKeyData) * nkeys); + INJECTION_POINT("inplace-before-pin"); + scan = systable_beginscan(relation, indexId, indexOK, snapshot, + nkeys, mutable_key); + oldtup = systable_getnext(scan); + if (!HeapTupleIsValid(oldtup)) + { + systable_endscan(scan); + *oldtupcopy = NULL; + return; + } + +#ifdef USE_ASSERT_CHECKING + if (RelationGetRelid(relation) == RelationRelationId) + check_inplace_rel_lock(oldtup); +#endif + } while (!inplace_xmax_lock(scan)); + + *oldtupcopy = heap_copytuple(oldtup); + *state = scan; +} + +/* + * heap_inplace_update_finish - second phase of heap_inplace_update_scan() + * + * The tuple cannot change size, and therefore its header fields and null + * bitmap (if any) don't change either. + */ +void +heap_inplace_update_finish(void *state, HeapTuple tuple) +{ + SysScanDesc scan = (SysScanDesc) state; + TupleTableSlot *slot = scan->slot; + BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot; + HeapTuple oldtup = bslot->base.tuple; + HeapTupleHeader htup = oldtup->t_data; + Buffer buffer = bslot->buffer; + Relation relation = scan->heap_rel; + uint32 oldlen; + uint32 newlen; + + Assert(ItemPointerEquals(&oldtup->t_self, &tuple->t_self)); + oldlen = oldtup->t_len - htup->t_hoff; newlen = tuple->t_len - tuple->t_data->t_hoff; if (oldlen != newlen || htup->t_hoff != tuple->t_data->t_hoff) elog(ERROR, "wrong tuple length"); @@ -6104,6 +6207,19 @@ heap_inplace_update(Relation relation, HeapTuple tuple) (char *) tuple->t_data + tuple->t_data->t_hoff, newlen); + /*---------- + * XXX A crash here can allow datfrozenxid() to get ahead of relfrozenxid: + * + * ["D" is a VACUUM (ONLY_DATABASE_STATS)] + * ["R" is a VACUUM tbl] + * D: vac_update_datfrozenid() -> systable_beginscan(pg_class) + * D: systable_getnext() returns pg_class tuple of tbl + * R: memcpy() into pg_class tuple of tbl + * D: raise pg_database.datfrozenxid, XLogInsert(), finish + * [crash] + * [recovery restores datfrozenxid w/o relfrozenxid] + */ + MarkBufferDirty(buffer); /* XLOG stuff */ @@ -6124,23 +6240,188 @@ heap_inplace_update(Relation relation, HeapTuple tuple) recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_INPLACE); - PageSetLSN(page, recptr); + PageSetLSN(BufferGetPage(buffer), recptr); } END_CRIT_SECTION(); - UnlockReleaseBuffer(buffer); + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + systable_endscan(scan); /* * Send out shared cache inval if necessary. Note that because we only * pass the new version of the tuple, this mustn't be used for any * operations that could change catcache lookup keys. But we aren't * bothering with index updates either, so that's true a fortiori. + * + * XXX ROLLBACK discards the invalidation. See test inplace-inval.spec. */ if (!IsBootstrapProcessingMode()) CacheInvalidateHeapTuple(relation, tuple, NULL); } +/* + * heap_inplace_update_cancel - abandon a heap_inplace_update_scan() + * + * This is an alternative to making a no-op update. + */ +void +heap_inplace_update_cancel(void *state) +{ + SysScanDesc scan = (SysScanDesc) state; + TupleTableSlot *slot = scan->slot; + BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot; + Buffer buffer = bslot->buffer; + + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + systable_endscan(scan); +} + +/* + * inplace_xmax_lock - protect inplace update from concurrent heap_update() + * + * This operates on the last tuple that systable_getnext() returned. Evaluate + * whether the tuple's state is compatible with a no-key update. Current + * transaction rowmarks are fine, as is KEY SHARE from any transaction. If + * compatible, return true with the buffer exclusive-locked. Otherwise, + * return false after blocking transactions, if any, have ended. + * + * One could modify this to return true for tuples with delete in progress, + * All inplace updaters take lock that conflicts with DROP. If it does happen + * somehow, we'll wait for it like we would an update. + * + * Readers of inplace-updated fields expect changes to those fields are + * durable. For example, vac_truncate_clog() reads datfrozenxid from + * pg_database tuples via catalog snapshots. A future snapshot must not + * return a lower datfrozenxid for the same database OID (lower in the + * FullTransactionIdPrecedes() sense). We achieve that since no update of a + * tuple can start while we hold a lock on its buffer. In cases like + * BEGIN;GRANT;CREATE INDEX;COMMIT we're inplace-updating a tuple visible only + * to this transaction. ROLLBACK then is one case where it's okay to lose + * inplace updates. (Restoring relhasindex=false on ROLLBACK is fine, since + * any concurrent CREATE INDEX would have blocked, then inplace-updated the + * committed tuple.) + * + * In principle, we could avoid waiting by overwriting every tuple in the + * updated tuple chain. Reader expectations permit updating a tuple only if + * it's aborted, is the tail of the chain, or we already updated the tuple + * referenced in its t_ctid. Hence, we would need to overwrite the tuples in + * order from tail to head. That would tolerate either (a) mutating all + * tuples in one critical section or (b) accepting a chance of partial + * completion. Partial completion of a relfrozenxid update would have the + * weird consequence that the table's next VACUUM could see the table's + * relfrozenxid move forward between vacuum_get_cutoffs() and finishing. + */ +static bool +inplace_xmax_lock(SysScanDesc scan) +{ + TupleTableSlot *slot = scan->slot; + BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot; + HeapTupleData oldtup = *bslot->base.tuple; + Buffer buffer = bslot->buffer; + TM_Result result; + bool ret; + + Assert(TTS_IS_BUFFERTUPLE(slot)); + Assert(BufferIsValid(buffer)); + + LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); + + /*---------- + * Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except: + * + * - wait unconditionally + * - no tuple locks + * - don't recheck header after wait: simpler to defer to next iteration + * - don't try to continue even if the updater aborts: likewise + * - no crosscheck + */ + result = HeapTupleSatisfiesUpdate(&oldtup, GetCurrentCommandId(false), + buffer); + + if (result == TM_Invisible) + { + /* no known way this can happen */ + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg_internal("attempted to overwrite invisible tuple"))); + } + else if (result == TM_SelfModified) + { + /* + * CREATE INDEX might reach this if an expression is silly enough to + * call e.g. SELECT ... FROM pg_class FOR SHARE. + */ + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("tuple to be updated was already modified by an operation triggered by the current command"))); + } + else if (result == TM_BeingModified) + { + TransactionId xwait; + uint16 infomask; + Relation relation; + + xwait = HeapTupleHeaderGetRawXmax(oldtup.t_data); + infomask = oldtup.t_data->t_infomask; + relation = scan->heap_rel; + + if (infomask & HEAP_XMAX_IS_MULTI) + { + LockTupleMode lockmode = LockTupleNoKeyExclusive; + MultiXactStatus mxact_status = MultiXactStatusNoKeyUpdate; + int remain; + bool current_is_member; + + if (DoesMultiXactIdConflict((MultiXactId) xwait, infomask, + lockmode, ¤t_is_member)) + { + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + systable_endscan(scan); + ret = false; + MultiXactIdWait((MultiXactId) xwait, mxact_status, infomask, + relation, &oldtup.t_self, XLTW_Update, + &remain); + } + else + ret = true; + } + else if (TransactionIdIsCurrentTransactionId(xwait)) + ret = true; + else if (HEAP_XMAX_IS_KEYSHR_LOCKED(infomask)) + ret = true; + else + { + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + systable_endscan(scan); + ret = false; + XactLockTableWait(xwait, relation, &oldtup.t_self, + XLTW_Update); + } + } + else + { + ret = (result == TM_Ok); + if (!ret) + { + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + systable_endscan(scan); + } + } + + /* + * GetCatalogSnapshot() relies on invalidation messages to know when to + * take a new snapshot. COMMIT of xwait is responsible for sending the + * invalidation. We're not acquiring heavyweight locks sufficient to + * block if not yet sent, so we must take a new snapshot to avoid spinning + * that ends with a "too many tries" error. While we don't need this if + * xwait aborted, don't bother optimizing that. + */ + if (!ret) + InvalidateCatalogSnapshot(); + return ret; +} + #define FRM_NOOP 0x0001 #define FRM_INVALIDATE_XMAX 0x0002 #define FRM_RETURN_IS_XID 0x0004 diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 5c48e57..00b3e4f 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2788,7 +2788,9 @@ index_update_stats(Relation rel, { Oid relid = RelationGetRelid(rel); Relation pg_class; + ScanKeyData key[1]; HeapTuple tuple; + void *state; Form_pg_class rd_rel; bool dirty; @@ -2822,33 +2824,12 @@ index_update_stats(Relation rel, pg_class = table_open(RelationRelationId, RowExclusiveLock); - /* - * Make a copy of the tuple to update. Normally we use the syscache, but - * we can't rely on that during bootstrap or while reindexing pg_class - * itself. - */ - if (IsBootstrapProcessingMode() || - ReindexIsProcessingHeap(RelationRelationId)) - { - /* don't assume syscache will work */ - TableScanDesc pg_class_scan; - ScanKeyData key[1]; - - ScanKeyInit(&key[0], - Anum_pg_class_oid, - BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(relid)); - - pg_class_scan = table_beginscan_catalog(pg_class, 1, key); - tuple = heap_getnext(pg_class_scan, ForwardScanDirection); - tuple = heap_copytuple(tuple); - table_endscan(pg_class_scan); - } - else - { - /* normal case, use syscache */ - tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid)); - } + ScanKeyInit(&key[0], + Anum_pg_class_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(relid)); + heap_inplace_update_scan(pg_class, ClassOidIndexId, true, NULL, 1, key, + &tuple, &state); if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for relation %u", relid); @@ -2911,11 +2892,12 @@ index_update_stats(Relation rel, */ if (dirty) { - heap_inplace_update(pg_class, tuple); + heap_inplace_update_finish(state, tuple); /* the above sends a cache inval message */ } else { + heap_inplace_update_cancel(state); /* no need to change tuple, but force relcache inval anyway */ CacheInvalidateRelcacheByTuple(tuple); } diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c index 738bc46..c882f3c 100644 --- a/src/backend/catalog/toasting.c +++ b/src/backend/catalog/toasting.c @@ -29,6 +29,7 @@ #include "catalog/toasting.h" #include "miscadmin.h" #include "nodes/makefuncs.h" +#include "utils/fmgroids.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -333,21 +334,36 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid, */ class_rel = table_open(RelationRelationId, RowExclusiveLock); - reltup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid)); - if (!HeapTupleIsValid(reltup)) - elog(ERROR, "cache lookup failed for relation %u", relOid); - - ((Form_pg_class) GETSTRUCT(reltup))->reltoastrelid = toast_relid; - if (!IsBootstrapProcessingMode()) { /* normal case, use a transactional update */ + reltup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid)); + if (!HeapTupleIsValid(reltup)) + elog(ERROR, "cache lookup failed for relation %u", relOid); + + ((Form_pg_class) GETSTRUCT(reltup))->reltoastrelid = toast_relid; + CatalogTupleUpdate(class_rel, &reltup->t_self, reltup); } else { /* While bootstrapping, we cannot UPDATE, so overwrite in-place */ - heap_inplace_update(class_rel, reltup); + + ScanKeyData key[1]; + void *state; + + ScanKeyInit(&key[0], + Anum_pg_class_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(relOid)); + heap_inplace_update_scan(class_rel, ClassOidIndexId, true, + NULL, 1, key, &reltup, &state); + if (!HeapTupleIsValid(reltup)) + elog(ERROR, "cache lookup failed for relation %u", relOid); + + ((Form_pg_class) GETSTRUCT(reltup))->reltoastrelid = toast_relid; + + heap_inplace_update_finish(state, reltup); } heap_freetuple(reltup); diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index be629ea..da4d2b7 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1637,6 +1637,8 @@ dropdb(const char *dbname, bool missing_ok, bool force) bool db_istemplate; Relation pgdbrel; HeapTuple tup; + ScanKeyData key[1]; + void *inplace_state; Form_pg_database datform; int notherbackends; int npreparedxacts; @@ -1774,11 +1776,6 @@ dropdb(const char *dbname, bool missing_ok, bool force) */ pgstat_drop_database(db_id); - tup = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(db_id)); - if (!HeapTupleIsValid(tup)) - elog(ERROR, "cache lookup failed for database %u", db_id); - datform = (Form_pg_database) GETSTRUCT(tup); - /* * Except for the deletion of the catalog row, subsequent actions are not * transactional (consider DropDatabaseBuffers() discarding modified @@ -1790,8 +1787,17 @@ dropdb(const char *dbname, bool missing_ok, bool force) * modification is durable before performing irreversible filesystem * operations. */ + ScanKeyInit(&key[0], + Anum_pg_database_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(db_id)); + heap_inplace_update_scan(pgdbrel, DatabaseOidIndexId, true, + NULL, 1, key, &tup, &inplace_state); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for database %u", db_id); + datform = (Form_pg_database) GETSTRUCT(tup); datform->datconnlimit = DATCONNLIMIT_INVALID_DB; - heap_inplace_update(pgdbrel, tup); + heap_inplace_update_finish(inplace_state, tup); XLogFlush(XactLastRecEnd); /* @@ -1799,6 +1805,7 @@ dropdb(const char *dbname, bool missing_ok, bool force) * the row will be gone, but if we fail, dropdb() can be invoked again. */ CatalogTupleDelete(pgdbrel, &tup->t_self); + heap_freetuple(tup); /* * Drop db-specific replication slots. diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c index 7a5ed6b..22d0ce7 100644 --- a/src/backend/commands/event_trigger.c +++ b/src/backend/commands/event_trigger.c @@ -946,25 +946,18 @@ EventTriggerOnLogin(void) { Relation pg_db = table_open(DatabaseRelationId, RowExclusiveLock); HeapTuple tuple; + void *state; Form_pg_database db; ScanKeyData key[1]; - SysScanDesc scan; - /* - * Get the pg_database tuple to scribble on. Note that this does - * not directly rely on the syscache to avoid issues with - * flattened toast values for the in-place update. - */ + /* Fetch a copy of the tuple to scribble on */ ScanKeyInit(&key[0], Anum_pg_database_oid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(MyDatabaseId)); - scan = systable_beginscan(pg_db, DatabaseOidIndexId, true, - NULL, 1, key); - tuple = systable_getnext(scan); - tuple = heap_copytuple(tuple); - systable_endscan(scan); + heap_inplace_update_scan(pg_db, DatabaseOidIndexId, true, + NULL, 1, key, &tuple, &state); if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for database %u", MyDatabaseId); @@ -980,13 +973,15 @@ EventTriggerOnLogin(void) * that avoids possible waiting on the row-level lock. Second, * that avoids dealing with TOAST. * - * It's known that changes made by heap_inplace_update() may - * be lost due to concurrent normal updates. However, we are - * OK with that. The subsequent connections will still have a - * chance to set "dathasloginevt" to false. + * Changes made by inplace update may be lost due to + * concurrent normal updates; see inplace-inval.spec. However, + * we are OK with that. The subsequent connections will still + * have a chance to set "dathasloginevt" to false. */ - heap_inplace_update(pg_db, tuple); + heap_inplace_update_finish(state, tuple); } + else + heap_inplace_update_cancel(state); table_close(pg_db, RowExclusiveLock); heap_freetuple(tuple); } diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 521ee74..64b9e9d 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -1405,7 +1405,9 @@ vac_update_relstats(Relation relation, { Oid relid = RelationGetRelid(relation); Relation rd; + ScanKeyData key[1]; HeapTuple ctup; + void *inplace_state; Form_pg_class pgcform; bool dirty, futurexid, @@ -1416,7 +1418,12 @@ vac_update_relstats(Relation relation, rd = table_open(RelationRelationId, RowExclusiveLock); /* Fetch a copy of the tuple to scribble on */ - ctup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid)); + ScanKeyInit(&key[0], + Anum_pg_class_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(relid)); + heap_inplace_update_scan(rd, ClassOidIndexId, true, + NULL, 1, key, &ctup, &inplace_state); if (!HeapTupleIsValid(ctup)) elog(ERROR, "pg_class entry for relid %u vanished during vacuuming", relid); @@ -1524,7 +1531,9 @@ vac_update_relstats(Relation relation, /* If anything changed, write out the tuple. */ if (dirty) - heap_inplace_update(rd, ctup); + heap_inplace_update_finish(inplace_state, ctup); + else + heap_inplace_update_cancel(inplace_state); table_close(rd, RowExclusiveLock); @@ -1576,6 +1585,7 @@ vac_update_datfrozenxid(void) bool bogus = false; bool dirty = false; ScanKeyData key[1]; + void *inplace_state; /* * Restrict this task to one backend per database. This avoids race @@ -1699,20 +1709,18 @@ vac_update_datfrozenxid(void) relation = table_open(DatabaseRelationId, RowExclusiveLock); /* - * Get the pg_database tuple to scribble on. Note that this does not - * directly rely on the syscache to avoid issues with flattened toast - * values for the in-place update. + * Fetch a copy of the tuple to scribble on. We could check the syscache + * tuple first. If that concluded !dirty, we'd avoid waiting on + * concurrent heap_update() and would avoid exclusive-locking the buffer. + * For now, don't optimize that. */ ScanKeyInit(&key[0], Anum_pg_database_oid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(MyDatabaseId)); - scan = systable_beginscan(relation, DatabaseOidIndexId, true, - NULL, 1, key); - tuple = systable_getnext(scan); - tuple = heap_copytuple(tuple); - systable_endscan(scan); + heap_inplace_update_scan(relation, DatabaseOidIndexId, true, + NULL, 1, key, &tuple, &inplace_state); if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for database %u", MyDatabaseId); @@ -1746,7 +1754,9 @@ vac_update_datfrozenxid(void) newMinMulti = dbform->datminmxid; if (dirty) - heap_inplace_update(relation, tuple); + heap_inplace_update_finish(inplace_state, tuple); + else + heap_inplace_update_cancel(inplace_state); heap_freetuple(tuple); table_close(relation, RowExclusiveLock); diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index c47a504..33e7134 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -336,7 +336,14 @@ extern TM_Result heap_lock_tuple(Relation relation, HeapTuple tuple, bool follow_updates, Buffer *buffer, struct TM_FailureData *tmfd); -extern void heap_inplace_update(Relation relation, HeapTuple tuple); +extern void heap_inplace_update_scan(Relation relation, + Oid indexId, + bool indexOK, + Snapshot snapshot, + int nkeys, const ScanKeyData *key, + HeapTuple *oldtupcopy, void **state); +extern void heap_inplace_update_finish(void *state, HeapTuple tuple); +extern void heap_inplace_update_cancel(void *state); extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple, const struct VacuumCutoffs *cutoffs, HeapPageFreeze *pagefrz, diff --git a/src/test/isolation/expected/intra-grant-inplace-db.out b/src/test/isolation/expected/intra-grant-inplace-db.out index 432ece5..a91402c 100644 --- a/src/test/isolation/expected/intra-grant-inplace-db.out +++ b/src/test/isolation/expected/intra-grant-inplace-db.out @@ -9,20 +9,20 @@ step b1: BEGIN; step grant1: GRANT TEMP ON DATABASE isolation_regression TO regress_temp_grantee; -step vac2: VACUUM (FREEZE); +step vac2: VACUUM (FREEZE); step snap3: INSERT INTO frozen_witness SELECT datfrozenxid FROM pg_database WHERE datname = current_catalog; step c1: COMMIT; +step vac2: <... completed> step cmp3: SELECT 'datfrozenxid retreated' FROM pg_database WHERE datname = current_catalog AND age(datfrozenxid) > (SELECT min(age(x)) FROM frozen_witness); -?column? ----------------------- -datfrozenxid retreated -(1 row) +?column? +-------- +(0 rows) diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out index cc1e47a..c2a9841 100644 --- a/src/test/isolation/expected/intra-grant-inplace.out +++ b/src/test/isolation/expected/intra-grant-inplace.out @@ -14,15 +14,16 @@ relhasindex f (1 row) -step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); step c1: COMMIT; +step addk2: <... completed> step read2: SELECT relhasindex FROM pg_class WHERE oid = 'intra_grant_inplace'::regclass; relhasindex ----------- -f +t (1 row) @@ -58,8 +59,9 @@ relhasindex f (1 row) -step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); step r3: ROLLBACK; +step addk2: <... completed> starting permutation: b2 sfnku2 addk2 c2 step b2: BEGIN; @@ -122,17 +124,18 @@ relhasindex f (1 row) -step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); step r3: ROLLBACK; step grant1: <... completed> step c1: COMMIT; +step addk2: <... completed> step read2: SELECT relhasindex FROM pg_class WHERE oid = 'intra_grant_inplace'::regclass; relhasindex ----------- -f +t (1 row) diff --git a/src/test/isolation/specs/intra-grant-inplace-db.spec b/src/test/isolation/specs/intra-grant-inplace-db.spec index bbecd5d..9de40ec 100644 --- a/src/test/isolation/specs/intra-grant-inplace-db.spec +++ b/src/test/isolation/specs/intra-grant-inplace-db.spec @@ -42,5 +42,4 @@ step cmp3 { } -# XXX extant bug permutation snap3 b1 grant1 vac2(c1) snap3 c1 cmp3 diff --git a/src/test/isolation/specs/intra-grant-inplace.spec b/src/test/isolation/specs/intra-grant-inplace.spec index 3cd696b..eed0b52 100644 --- a/src/test/isolation/specs/intra-grant-inplace.spec +++ b/src/test/isolation/specs/intra-grant-inplace.spec @@ -73,7 +73,7 @@ step keyshr5 { teardown { ROLLBACK; } -# XXX extant bugs: permutation comments refer to planned post-bugfix behavior +# XXX extant bugs: permutation comments refer to planned future LockTuple() permutation b1 diff --git a/src/test/modules/injection_points/expected/inplace.out b/src/test/modules/injection_points/expected/inplace.out index 123f45a..db7dab6 100644 --- a/src/test/modules/injection_points/expected/inplace.out +++ b/src/test/modules/injection_points/expected/inplace.out @@ -40,4 +40,301 @@ step read1: SELECT reltuples = -1 AS reltuples_unknown FROM pg_class WHERE oid = 'vactest.orig50'::regclass; -ERROR: could not create unique index "pg_class_oid_index" +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: begin2 grant2 vac1 c2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step c2: COMMIT; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: begin2 grant2 vac1 r2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step r2: ROLLBACK; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: begin2 grant2 vac1 c2 revoke2 grant2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step c2: COMMIT; +step revoke2: REVOKE SELECT ON TABLE vactest.orig50 FROM PUBLIC; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: vac1 begin2 grant2 revoke2 mkrels3 c2 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step revoke2: REVOKE SELECT ON TABLE vactest.orig50 FROM PUBLIC; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step c2: COMMIT; +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: begin2 grant2 vac1 r2 grant2 revoke2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step r2: ROLLBACK; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step revoke2: REVOKE SELECT ON TABLE vactest.orig50 FROM PUBLIC; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + + +starting permutation: begin2 grant2 vac1 c2 revoke2 vac3 mkrels3 read1 +mkrels +------ + +(1 row) + +injection_points_attach +----------------------- + +(1 row) + +step begin2: BEGIN; +step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; +step vac1: VACUUM vactest.orig50; -- wait during inplace update +step c2: COMMIT; +step revoke2: REVOKE SELECT ON TABLE vactest.orig50 FROM PUBLIC; +step vac3: VACUUM pg_class; +step mkrels3: + SELECT vactest.mkrels('intruder', 1, 100); -- repopulate LP_UNUSED + SELECT injection_points_detach('inplace-before-pin'); + SELECT injection_points_wakeup('inplace-before-pin'); + +mkrels +------ + +(1 row) + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step vac1: <... completed> +step read1: + REINDEX TABLE pg_class; -- look for duplicates + SELECT reltuples = -1 AS reltuples_unknown + FROM pg_class WHERE oid = 'vactest.orig50'::regclass; + +reltuples_unknown +----------------- +f +(1 row) + diff --git a/src/test/modules/injection_points/specs/inplace.spec b/src/test/modules/injection_points/specs/inplace.spec index e957713..86539a5 100644 --- a/src/test/modules/injection_points/specs/inplace.spec +++ b/src/test/modules/injection_points/specs/inplace.spec @@ -32,12 +32,9 @@ setup CREATE TABLE vactest.orig50 (); SELECT vactest.mkrels('orig', 51, 100); } - -# XXX DROP causes an assertion failure; adopt DROP once fixed teardown { - --DROP SCHEMA vactest CASCADE; - DO $$BEGIN EXECUTE 'ALTER SCHEMA vactest RENAME TO schema' || oid FROM pg_namespace where nspname = 'vactest'; END$$; + DROP SCHEMA vactest CASCADE; DROP EXTENSION injection_points; } @@ -56,11 +53,13 @@ step read1 { FROM pg_class WHERE oid = 'vactest.orig50'::regclass; } - # Transactional updates of the tuple vac1 is waiting to inplace-update. session s2 step grant2 { GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; } - +step revoke2 { REVOKE SELECT ON TABLE vactest.orig50 FROM PUBLIC; } +step begin2 { BEGIN; } +step c2 { COMMIT; } +step r2 { ROLLBACK; } # Non-blocking actions. session s3 @@ -74,10 +73,69 @@ step mkrels3 { } -# XXX extant bug +# target gains a successor at the last moment permutation vac1(mkrels3) # reads pg_class tuple T0 for vactest.orig50, xmax invalid grant2 # T0 becomes eligible for pruning, T1 is successor vac3 # T0 becomes LP_UNUSED - mkrels3 # T0 reused; vac1 wakes and overwrites the reused T0 + mkrels3 # vac1 wakes, scans to T1 read1 + +# target already has a successor, which commits +permutation + begin2 + grant2 # T0.t_ctid = T1 + vac1(mkrels3) # reads T0 for vactest.orig50 + c2 # T0 becomes eligible for pruning + vac3 # T0 becomes LP_UNUSED + mkrels3 # vac1 wakes, scans to T1 + read1 + +# target already has a successor, which becomes LP_UNUSED at the last moment +permutation + begin2 + grant2 # T0.t_ctid = T1 + vac1(mkrels3) # reads T0 for vactest.orig50 + r2 # T1 becomes eligible for pruning + vac3 # T1 becomes LP_UNUSED + mkrels3 # reuse T1; vac1 scans to T0 + read1 + +# target already has a successor, which becomes LP_REDIRECT at the last moment +permutation + begin2 + grant2 # T0.t_ctid = T1, non-HOT due to filled page + vac1(mkrels3) # reads T0 + c2 + revoke2 # HOT update to T2 + grant2 # HOT update to T3 + vac3 # T1 becomes LP_REDIRECT + mkrels3 # reuse T2; vac1 scans to T3 + read1 + +# waiting for updater to end +permutation + vac1(c2) # reads pg_class tuple T0 for vactest.orig50, xmax invalid + begin2 + grant2 # T0.t_ctid = T1, non-HOT due to filled page + revoke2 # HOT update to T2 + mkrels3 # vac1 awakes briefly, then waits for s2 + c2 + read1 + +# Another LP_UNUSED. This time, do change the live tuple. Final live tuple +# body is identical to original, at a different TID. +permutation + begin2 + grant2 # T0.t_ctid = T1, non-HOT due to filled page + vac1(mkrels3) # reads T0 + r2 # T1 becomes eligible for pruning + grant2 # T0.t_ctid = T2; T0 becomes eligible for pruning + revoke2 # T2.t_ctid = T3; T2 becomes eligible for pruning + vac3 # T0, T1 & T2 become LP_UNUSED + mkrels3 # reuse T0, T1 & T2; vac1 scans to T3 + read1 + +# Another LP_REDIRECT. Compared to the earlier test, omit the last grant2. +# Hence, final live tuple body is identical to original, at a different TID. +permutation begin2 grant2 vac1(mkrels3) c2 revoke2 vac3 mkrels3 read1 --3pz3+3yBgJC2Q0yV Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="inplace120-locktag-v2.patch" Author: Noah Misch Commit: Noah Misch Make heap_update() callers wait for inplace update. The previous commit fixed some ways of losing an inplace update. It remained possible to lose one when a backend working toward a heap_update() copied a tuple into memory just before inplace update of that tuple. In catalogs eligible for inplace update, use LOCKTAG_TUPLE to govern admission to the steps of copying an old tuple, modifying it, and issuing heap_update(). This includes UPDATE and MERGE commands. To avoid changing most of the pg_class DDL, don't require LOCKTAG_TUPLE when holding a relation lock sufficient to exclude inplace updaters. Back-patch to v12 (all supported versions). Reviewed by FIXME. Discussion: https://postgr.es/m/20231027214946.79.nmisch@google.com diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock index dbfa2b7..fb06ff2 100644 --- a/src/backend/access/heap/README.tuplock +++ b/src/backend/access/heap/README.tuplock @@ -157,8 +157,6 @@ is set. Locking to write inplace-updated tables --------------------------------------- -[This is the plan, but LOCKTAG_TUPLE acquisition is not yet here.] - If IsInplaceUpdateRelation() returns true for a table, the table is a system catalog that receives heap_inplace_update_scan() calls. Preparing a heap_update() of these tables follows additional locking rules, to ensure we diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index f1d4fc0..248af401 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -51,6 +51,8 @@ #include "access/xloginsert.h" #include "access/xlogutils.h" #include "catalog/catalog.h" +#include "catalog/pg_database.h" +#include "catalog/pg_database_d.h" #include "commands/vacuum.h" #include "miscadmin.h" #include "pgstat.h" @@ -77,6 +79,9 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared); #ifdef USE_ASSERT_CHECKING +static void check_lock_if_inplace_updateable_rel(Relation relation, + ItemPointer otid, + HeapTuple newtup); static void check_inplace_rel_lock(HeapTuple oldtup); #endif static Bitmapset *HeapDetermineColumnsInfo(Relation relation, @@ -126,6 +131,8 @@ static HeapTuple ExtractReplicaIdentity(Relation relation, HeapTuple tp, bool ke * heavyweight lock mode and MultiXactStatus values to use for any particular * tuple lock strength. * + * These interact with InplaceUpdateTupleLock, an alias for ExclusiveLock. + * * Don't look at lockstatus/updstatus directly! Use get_mxact_status_for_lock * instead. */ @@ -3209,6 +3216,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, (errcode(ERRCODE_INVALID_TRANSACTION_STATE), errmsg("cannot update tuples during a parallel operation"))); +#ifdef USE_ASSERT_CHECKING + check_lock_if_inplace_updateable_rel(relation, otid, newtup); +#endif + /* * Fetch the list of attributes to be checked for various operations. * @@ -4075,6 +4086,89 @@ l2: #ifdef USE_ASSERT_CHECKING /* + * Confirm adequate lock held during heap_update(), per rules from + * README.tuplock section "Locking to write inplace-updated tables". + */ +static void +check_lock_if_inplace_updateable_rel(Relation relation, + ItemPointer otid, + HeapTuple newtup) +{ + /* LOCKTAG_TUPLE acceptable for any catalog */ + switch (RelationGetRelid(relation)) + { + case RelationRelationId: + case DatabaseRelationId: + { + LOCKTAG tuptag; + + SET_LOCKTAG_TUPLE(tuptag, + relation->rd_lockInfo.lockRelId.dbId, + relation->rd_lockInfo.lockRelId.relId, + ItemPointerGetBlockNumber(otid), + ItemPointerGetOffsetNumber(otid)); + if (LockHeldByMe(&tuptag, InplaceUpdateTupleLock, false)) + return; + } + break; + default: + Assert(!IsInplaceUpdateRelation(relation)); + return; + } + + switch (RelationGetRelid(relation)) + { + case RelationRelationId: + { + /* LOCKTAG_TUPLE or LOCKTAG_RELATION ok */ + Form_pg_class classForm = (Form_pg_class) GETSTRUCT(newtup); + Oid relid = classForm->oid; + Oid dbid; + LOCKTAG tag; + + if (IsSharedRelation(relid)) + dbid = InvalidOid; + else + dbid = MyDatabaseId; + + if (classForm->relkind == RELKIND_INDEX) + { + Relation irel = index_open(relid, AccessShareLock); + + SET_LOCKTAG_RELATION(tag, dbid, irel->rd_index->indrelid); + index_close(irel, AccessShareLock); + } + else + SET_LOCKTAG_RELATION(tag, dbid, relid); + + if (!LockHeldByMe(&tag, ShareUpdateExclusiveLock, false) && + !LockHeldByMe(&tag, ShareRowExclusiveLock, true)) + elog(WARNING, + "missing lock for relation \"%s\" (OID %u, relkind %c) @ TID (%u,%u)", + NameStr(classForm->relname), + relid, + classForm->relkind, + ItemPointerGetBlockNumber(otid), + ItemPointerGetOffsetNumber(otid)); + } + break; + case DatabaseRelationId: + { + /* LOCKTAG_TUPLE required */ + Form_pg_database dbForm = (Form_pg_database) GETSTRUCT(newtup); + + elog(WARNING, + "missing lock on database \"%s\" (OID %u) @ TID (%u,%u)", + NameStr(dbForm->datname), + dbForm->oid, + ItemPointerGetBlockNumber(otid), + ItemPointerGetOffsetNumber(otid)); + } + break; + } +} + +/* * Confirm adequate relation lock held, per rules from README.tuplock section * "Locking to write inplace-updated tables". */ @@ -6120,6 +6214,7 @@ heap_inplace_update_scan(Relation relation, int retries = 0; SysScanDesc scan; HeapTuple oldtup; + ItemPointerData locked; /* * For now, we don't allow parallel updates. Unlike a regular update, @@ -6141,6 +6236,7 @@ heap_inplace_update_scan(Relation relation, Assert(IsInplaceUpdateRelation(relation) || !IsSystemRelation(relation)); /* Loop for an exclusive-locked buffer of a non-updated tuple. */ + ItemPointerSetInvalid(&locked); do { CHECK_FOR_INTERRUPTS(); @@ -6160,6 +6256,8 @@ heap_inplace_update_scan(Relation relation, oldtup = systable_getnext(scan); if (!HeapTupleIsValid(oldtup)) { + if (ItemPointerIsValid(&locked)) + UnlockTuple(relation, &locked, InplaceUpdateTupleLock); systable_endscan(scan); *oldtupcopy = NULL; return; @@ -6169,6 +6267,15 @@ heap_inplace_update_scan(Relation relation, if (RelationGetRelid(relation) == RelationRelationId) check_inplace_rel_lock(oldtup); #endif + + if (!(ItemPointerIsValid(&locked) && + ItemPointerEquals(&locked, &oldtup->t_self))) + { + if (ItemPointerIsValid(&locked)) + UnlockTuple(relation, &locked, InplaceUpdateTupleLock); + LockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock); + } + locked = oldtup->t_self; } while (!inplace_xmax_lock(scan)); *oldtupcopy = heap_copytuple(oldtup); @@ -6180,6 +6287,8 @@ heap_inplace_update_scan(Relation relation, * * The tuple cannot change size, and therefore its header fields and null * bitmap (if any) don't change either. + * + * Since we hold LOCKTAG_TUPLE, no updater has a local copy of this tuple. */ void heap_inplace_update_finish(void *state, HeapTuple tuple) @@ -6246,6 +6355,7 @@ heap_inplace_update_finish(void *state, HeapTuple tuple) END_CRIT_SECTION(); LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock); systable_endscan(scan); /* @@ -6271,9 +6381,12 @@ heap_inplace_update_cancel(void *state) SysScanDesc scan = (SysScanDesc) state; TupleTableSlot *slot = scan->slot; BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot; + HeapTuple oldtup = bslot->base.tuple; Buffer buffer = bslot->buffer; + Relation relation = scan->heap_rel; LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + UnlockTuple(relation, &oldtup->t_self, InplaceUpdateTupleLock); systable_endscan(scan); } @@ -6331,7 +6444,7 @@ inplace_xmax_lock(SysScanDesc scan) * Interpret HeapTupleSatisfiesUpdate() like heap_update() does, except: * * - wait unconditionally - * - no tuple locks + * - caller handles tuple lock, since inplace needs it unconditionally * - don't recheck header after wait: simpler to defer to next iteration * - don't try to continue even if the updater aborts: likewise * - no crosscheck diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 143876b..49d4d5e 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -75,6 +75,7 @@ #include "nodes/makefuncs.h" #include "parser/parse_func.h" #include "parser/parse_type.h" +#include "storage/lmgr.h" #include "utils/acl.h" #include "utils/aclchk_internal.h" #include "utils/builtins.h" @@ -1848,7 +1849,7 @@ ExecGrant_Relation(InternalGrant *istmt) HeapTuple tuple; ListCell *cell_colprivs; - tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid)); + tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relOid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for relation %u", relOid); pg_class_tuple = (Form_pg_class) GETSTRUCT(tuple); @@ -2060,6 +2061,7 @@ ExecGrant_Relation(InternalGrant *istmt) values, nulls, replaces); CatalogTupleUpdate(relation, &newtuple->t_self, newtuple); + UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock); /* Update initial privileges for extensions */ recordExtensionInitPriv(relOid, RelationRelationId, 0, @@ -2073,6 +2075,8 @@ ExecGrant_Relation(InternalGrant *istmt) pfree(new_acl); } + else + UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock); /* * Handle column-level privileges, if any were specified or implied. @@ -2186,7 +2190,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs, Oid *oldmembers; Oid *newmembers; - tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid)); + tuple = SearchSysCacheLocked1(cacheid, ObjectIdGetDatum(objectid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for %s %u", get_object_class_descr(classid), objectid); @@ -2262,6 +2266,7 @@ ExecGrant_common(InternalGrant *istmt, Oid classid, AclMode default_privs, nulls, replaces); CatalogTupleUpdate(relation, &newtuple->t_self, newtuple); + UnlockTuple(relation, &tuple->t_self, InplaceUpdateTupleLock); /* Update initial privileges for extensions */ recordExtensionInitPriv(objectid, classid, 0, ownerId, new_acl); diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 6c39434..8aefbcd 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -138,6 +138,15 @@ IsCatalogRelationOid(Oid relid) /* * IsInplaceUpdateRelation * True iff core code performs inplace updates on the relation. + * + * This is used for assertions and for making the executor follow the + * locking protocol described at README.tuplock section "Locking to write + * inplace-updated tables". Extensions may inplace-update other heap + * tables, but concurrent SQL UPDATE on the same table may overwrite + * those modifications. + * + * The executor can assume these are not partitions or partitioned and + * have no triggers. */ bool IsInplaceUpdateRelation(Relation relation) diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index da4d2b7..fd48022 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1864,6 +1864,7 @@ RenameDatabase(const char *oldname, const char *newname) { Oid db_id; HeapTuple newtup; + ItemPointerData otid; Relation rel; int notherbackends; int npreparedxacts; @@ -1935,11 +1936,13 @@ RenameDatabase(const char *oldname, const char *newname) errdetail_busy_db(notherbackends, npreparedxacts))); /* rename */ - newtup = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(db_id)); + newtup = SearchSysCacheLockedCopy1(DATABASEOID, ObjectIdGetDatum(db_id)); if (!HeapTupleIsValid(newtup)) elog(ERROR, "cache lookup failed for database %u", db_id); + otid = newtup->t_self; namestrcpy(&(((Form_pg_database) GETSTRUCT(newtup))->datname), newname); - CatalogTupleUpdate(rel, &newtup->t_self, newtup); + CatalogTupleUpdate(rel, &otid, newtup); + UnlockTuple(rel, &otid, InplaceUpdateTupleLock); InvokeObjectPostAlterHook(DatabaseRelationId, db_id, 0); @@ -2188,6 +2191,7 @@ movedb(const char *dbname, const char *tblspcname) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_DATABASE), errmsg("database \"%s\" does not exist", dbname))); + LockTuple(pgdbrel, &oldtuple->t_self, InplaceUpdateTupleLock); new_record[Anum_pg_database_dattablespace - 1] = ObjectIdGetDatum(dst_tblspcoid); new_record_repl[Anum_pg_database_dattablespace - 1] = true; @@ -2196,6 +2200,7 @@ movedb(const char *dbname, const char *tblspcname) new_record, new_record_nulls, new_record_repl); CatalogTupleUpdate(pgdbrel, &oldtuple->t_self, newtuple); + UnlockTuple(pgdbrel, &oldtuple->t_self, InplaceUpdateTupleLock); InvokeObjectPostAlterHook(DatabaseRelationId, db_id, 0); @@ -2426,6 +2431,7 @@ AlterDatabase(ParseState *pstate, AlterDatabaseStmt *stmt, bool isTopLevel) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_DATABASE), errmsg("database \"%s\" does not exist", stmt->dbname))); + LockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); datform = (Form_pg_database) GETSTRUCT(tuple); dboid = datform->oid; @@ -2475,6 +2481,7 @@ AlterDatabase(ParseState *pstate, AlterDatabaseStmt *stmt, bool isTopLevel) newtuple = heap_modify_tuple(tuple, RelationGetDescr(rel), new_record, new_record_nulls, new_record_repl); CatalogTupleUpdate(rel, &tuple->t_self, newtuple); + UnlockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); InvokeObjectPostAlterHook(DatabaseRelationId, dboid, 0); @@ -2524,6 +2531,7 @@ AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt) if (!object_ownercheck(DatabaseRelationId, db_id, GetUserId())) aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE, stmt->dbname); + LockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); datum = heap_getattr(tuple, Anum_pg_database_datcollversion, RelationGetDescr(rel), &isnull); oldversion = isnull ? NULL : TextDatumGetCString(datum); @@ -2552,6 +2560,7 @@ AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt) bool nulls[Natts_pg_database] = {0}; bool replaces[Natts_pg_database] = {0}; Datum values[Natts_pg_database] = {0}; + HeapTuple newtuple; ereport(NOTICE, (errmsg("changing version from %s to %s", @@ -2560,14 +2569,15 @@ AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt) values[Anum_pg_database_datcollversion - 1] = CStringGetTextDatum(newversion); replaces[Anum_pg_database_datcollversion - 1] = true; - tuple = heap_modify_tuple(tuple, RelationGetDescr(rel), - values, nulls, replaces); - CatalogTupleUpdate(rel, &tuple->t_self, tuple); - heap_freetuple(tuple); + newtuple = heap_modify_tuple(tuple, RelationGetDescr(rel), + values, nulls, replaces); + CatalogTupleUpdate(rel, &tuple->t_self, newtuple); + heap_freetuple(newtuple); } else ereport(NOTICE, (errmsg("version has not changed"))); + UnlockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); InvokeObjectPostAlterHook(DatabaseRelationId, db_id, 0); @@ -2679,6 +2689,8 @@ AlterDatabaseOwner(const char *dbname, Oid newOwnerId) (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("permission denied to change owner of database"))); + LockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); + repl_repl[Anum_pg_database_datdba - 1] = true; repl_val[Anum_pg_database_datdba - 1] = ObjectIdGetDatum(newOwnerId); @@ -2700,6 +2712,7 @@ AlterDatabaseOwner(const char *dbname, Oid newOwnerId) newtuple = heap_modify_tuple(tuple, RelationGetDescr(rel), repl_val, repl_null, repl_repl); CatalogTupleUpdate(rel, &newtuple->t_self, newtuple); + UnlockTuple(rel, &tuple->t_self, InplaceUpdateTupleLock); heap_freetuple(newtuple); diff --git a/src/backend/commands/event_trigger.c b/src/backend/commands/event_trigger.c index 22d0ce7..36d82bd 100644 --- a/src/backend/commands/event_trigger.c +++ b/src/backend/commands/event_trigger.c @@ -388,6 +388,7 @@ SetDatabaseHasLoginEventTriggers(void) /* Set dathasloginevt flag in pg_database */ Form_pg_database db; Relation pg_db = table_open(DatabaseRelationId, RowExclusiveLock); + ItemPointerData otid; HeapTuple tuple; /* @@ -399,16 +400,18 @@ SetDatabaseHasLoginEventTriggers(void) */ LockSharedObject(DatabaseRelationId, MyDatabaseId, 0, AccessExclusiveLock); - tuple = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId)); + tuple = SearchSysCacheLockedCopy1(DATABASEOID, ObjectIdGetDatum(MyDatabaseId)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for database %u", MyDatabaseId); + otid = tuple->t_self; db = (Form_pg_database) GETSTRUCT(tuple); if (!db->dathasloginevt) { db->dathasloginevt = true; - CatalogTupleUpdate(pg_db, &tuple->t_self, tuple); + CatalogTupleUpdate(pg_db, &otid, tuple); CommandCounterIncrement(); } + UnlockTuple(pg_db, &otid, InplaceUpdateTupleLock); table_close(pg_db, RowExclusiveLock); heap_freetuple(tuple); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 716e0e8..56c0c93 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -4529,14 +4529,17 @@ update_relispartition(Oid relationId, bool newval) { HeapTuple tup; Relation classRel; + ItemPointerData otid; classRel = table_open(RelationRelationId, RowExclusiveLock); - tup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relationId)); + tup = SearchSysCacheLockedCopy1(RELOID, ObjectIdGetDatum(relationId)); if (!HeapTupleIsValid(tup)) elog(ERROR, "cache lookup failed for relation %u", relationId); + otid = tup->t_self; Assert(((Form_pg_class) GETSTRUCT(tup))->relispartition != newval); ((Form_pg_class) GETSTRUCT(tup))->relispartition = newval; - CatalogTupleUpdate(classRel, &tup->t_self, tup); + CatalogTupleUpdate(classRel, &otid, tup); + UnlockTuple(classRel, &otid, InplaceUpdateTupleLock); heap_freetuple(tup); table_close(classRel, RowExclusiveLock); } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 7b66c57..b79980a 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3707,6 +3707,7 @@ SetRelationTableSpace(Relation rel, { Relation pg_class; HeapTuple tuple; + ItemPointerData otid; Form_pg_class rd_rel; Oid reloid = RelationGetRelid(rel); @@ -3715,9 +3716,10 @@ SetRelationTableSpace(Relation rel, /* Get a modifiable copy of the relation's pg_class row. */ pg_class = table_open(RelationRelationId, RowExclusiveLock); - tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid)); + tuple = SearchSysCacheLockedCopy1(RELOID, ObjectIdGetDatum(reloid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for relation %u", reloid); + otid = tuple->t_self; rd_rel = (Form_pg_class) GETSTRUCT(tuple); /* Update the pg_class row. */ @@ -3725,7 +3727,8 @@ SetRelationTableSpace(Relation rel, InvalidOid : newTableSpaceId; if (RelFileNumberIsValid(newRelFilenumber)) rd_rel->relfilenode = newRelFilenumber; - CatalogTupleUpdate(pg_class, &tuple->t_self, tuple); + CatalogTupleUpdate(pg_class, &otid, tuple); + UnlockTuple(pg_class, &otid, InplaceUpdateTupleLock); /* * Record dependency on tablespace. This is only required for relations @@ -4222,6 +4225,7 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo { Relation targetrelation; Relation relrelation; /* for RELATION relation */ + ItemPointerData otid; HeapTuple reltup; Form_pg_class relform; Oid namespaceId; @@ -4244,7 +4248,8 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo */ relrelation = table_open(RelationRelationId, RowExclusiveLock); - reltup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(myrelid)); + reltup = SearchSysCacheLockedCopy1(RELOID, ObjectIdGetDatum(myrelid)); + otid = reltup->t_self; if (!HeapTupleIsValid(reltup)) /* shouldn't happen */ elog(ERROR, "cache lookup failed for relation %u", myrelid); relform = (Form_pg_class) GETSTRUCT(reltup); @@ -4271,7 +4276,8 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo */ namestrcpy(&(relform->relname), newrelname); - CatalogTupleUpdate(relrelation, &reltup->t_self, reltup); + CatalogTupleUpdate(relrelation, &otid, reltup); + UnlockTuple(relrelation, &otid, InplaceUpdateTupleLock); InvokeObjectPostAlterHookArg(RelationRelationId, myrelid, 0, InvalidOid, is_internal); @@ -15644,7 +15650,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, /* Fetch heap tuple */ relid = RelationGetRelid(rel); - tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + tuple = SearchSysCacheLocked1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(tuple)) elog(ERROR, "cache lookup failed for relation %u", relid); @@ -15748,6 +15754,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, repl_val, repl_null, repl_repl); CatalogTupleUpdate(pgclass, &newtuple->t_self, newtuple); + UnlockTuple(pgclass, &tuple->t_self, InplaceUpdateTupleLock); InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0); @@ -18061,7 +18068,8 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid, ObjectAddress thisobj; bool already_done = false; - classTup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relOid)); + /* no rel lock for relkind=c so use LOCKTAG_TUPLE */ + classTup = SearchSysCacheLockedCopy1(RELOID, ObjectIdGetDatum(relOid)); if (!HeapTupleIsValid(classTup)) elog(ERROR, "cache lookup failed for relation %u", relOid); classForm = (Form_pg_class) GETSTRUCT(classTup); @@ -18080,6 +18088,8 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid, already_done = object_address_present(&thisobj, objsMoved); if (!already_done && oldNspOid != newNspOid) { + ItemPointerData otid = classTup->t_self; + /* check for duplicate name (more friendly than unique-index failure) */ if (get_relname_relid(NameStr(classForm->relname), newNspOid) != InvalidOid) @@ -18092,7 +18102,9 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid, /* classTup is a copy, so OK to scribble on */ classForm->relnamespace = newNspOid; - CatalogTupleUpdate(classRel, &classTup->t_self, classTup); + CatalogTupleUpdate(classRel, &otid, classTup); + UnlockTuple(classRel, &otid, InplaceUpdateTupleLock); + /* Update dependency on schema if caller said so */ if (hasDependEntry && @@ -18104,6 +18116,8 @@ AlterRelationNamespaceInternal(Relation classRel, Oid relOid, elog(ERROR, "could not change schema dependency for relation \"%s\"", NameStr(classForm->relname)); } + else + UnlockTuple(classRel, &classTup->t_self, InplaceUpdateTupleLock); if (!already_done) { add_exact_object_address(&thisobj, objsMoved); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4d7c92d..321ad47 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1209,6 +1209,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, resultRelInfo->ri_NumIndices = 0; resultRelInfo->ri_IndexRelationDescs = NULL; resultRelInfo->ri_IndexRelationInfo = NULL; + resultRelInfo->ri_needLockTagTuple = + IsInplaceUpdateRelation(resultRelationDesc); /* make a copy so as not to depend on relcache info not changing... */ resultRelInfo->ri_TrigDesc = CopyTriggerDesc(resultRelationDesc->trigdesc); if (resultRelInfo->ri_TrigDesc) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index d0a89cd..f18efdb 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -559,8 +559,12 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo, Relation rel = resultRelInfo->ri_RelationDesc; ItemPointer tid = &(searchslot->tts_tid); - /* For now we support only tables. */ + /* + * We support only non-system tables, with + * check_publication_add_relation() accountable. + */ Assert(rel->rd_rel->relkind == RELKIND_RELATION); + Assert(!IsCatalogRelation(rel)); CheckCmdReplicaIdentity(rel, CMD_UPDATE); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index a2442b7..b70d2f6 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2320,6 +2320,8 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo, } else { + ItemPointerData lockedtid; + /* * If we generate a new candidate tuple after EvalPlanQual testing, we * must loop back here to try again. (We don't need to redo triggers, @@ -2328,6 +2330,7 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo, * to do them again.) */ redo_act: + lockedtid = *tupleid; result = ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, slot, canSetTag, &updateCxt); @@ -2421,6 +2424,14 @@ redo_act: ExecInitUpdateProjection(context->mtstate, resultRelInfo); + if (resultRelInfo->ri_needLockTagTuple) + { + UnlockTuple(resultRelationDesc, + &lockedtid, InplaceUpdateTupleLock); + LockTuple(resultRelationDesc, + tupleid, InplaceUpdateTupleLock); + } + /* Fetch the most recent version of old tuple. */ oldSlot = resultRelInfo->ri_oldTupleSlot; if (!table_tuple_fetch_row_version(resultRelationDesc, @@ -2525,6 +2536,14 @@ ExecOnConflictUpdate(ModifyTableContext *context, TransactionId xmin; bool isnull; + /* + * Parse analysis should have blocked ON CONFLICT for all system + * relations, which includes these. There's no fundamental obstacle to + * supporting this; we'd just need to handle LOCKTAG_TUPLE like the other + * ExecUpdate() caller. + */ + Assert(!resultRelInfo->ri_needLockTagTuple); + /* Determine lock mode to use */ lockmode = ExecUpdateLockMode(context->estate, resultRelInfo); @@ -2850,6 +2869,7 @@ ExecMergeMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo, { ModifyTableState *mtstate = context->mtstate; List **mergeActions = resultRelInfo->ri_MergeActions; + ItemPointerData lockedtid; List *actionStates; TupleTableSlot *newslot = NULL; TupleTableSlot *rslot = NULL; @@ -2886,17 +2906,33 @@ ExecMergeMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo, * target wholerow junk attr. */ Assert(tupleid != NULL || oldtuple != NULL); + ItemPointerSetInvalid(&lockedtid); if (oldtuple != NULL) { Assert(resultRelInfo->ri_TrigDesc); + Assert(!resultRelInfo->ri_needLockTagTuple); ExecForceStoreHeapTuple(oldtuple, resultRelInfo->ri_oldTupleSlot, false); } - else if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc, - tupleid, - SnapshotAny, - resultRelInfo->ri_oldTupleSlot)) - elog(ERROR, "failed to fetch the target tuple"); + else + { + if (resultRelInfo->ri_needLockTagTuple) + { + /* + * This locks even tuples that don't match mas_whenqual, which + * isn't ideal. MERGE on system catalogs is a minor use case, so + * don't bother doing better. + */ + LockTuple(resultRelInfo->ri_RelationDesc, tupleid, + InplaceUpdateTupleLock); + lockedtid = *tupleid; + } + if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc, + tupleid, + SnapshotAny, + resultRelInfo->ri_oldTupleSlot)) + elog(ERROR, "failed to fetch the target tuple"); + } /* * Test the join condition. If it's satisfied, perform a MATCHED action. @@ -2968,7 +3004,7 @@ lmerge_matched: tupleid, NULL, newslot, &result)) { if (result == TM_Ok) - return NULL; /* "do nothing" */ + goto out; /* "do nothing" */ break; /* concurrent update/delete */ } @@ -2979,7 +3015,7 @@ lmerge_matched: { if (!ExecIRUpdateTriggers(estate, resultRelInfo, oldtuple, newslot)) - return NULL; /* "do nothing" */ + goto out; /* "do nothing" */ } else { @@ -2999,7 +3035,8 @@ lmerge_matched: if (updateCxt.crossPartUpdate) { mtstate->mt_merge_updated += 1; - return context->cpUpdateReturningSlot; + rslot = context->cpUpdateReturningSlot; + goto out; } } @@ -3017,7 +3054,7 @@ lmerge_matched: NULL, NULL, &result)) { if (result == TM_Ok) - return NULL; /* "do nothing" */ + goto out; /* "do nothing" */ break; /* concurrent update/delete */ } @@ -3028,7 +3065,7 @@ lmerge_matched: { if (!ExecIRDeleteTriggers(estate, resultRelInfo, oldtuple)) - return NULL; /* "do nothing" */ + goto out; /* "do nothing" */ } else result = ExecDeleteAct(context, resultRelInfo, tupleid, @@ -3109,7 +3146,7 @@ lmerge_matched: * let caller handle it under NOT MATCHED [BY TARGET] clauses. */ *matched = false; - return NULL; + goto out; case TM_Updated: { @@ -3183,7 +3220,7 @@ lmerge_matched: * more to do. */ if (TupIsNull(epqslot)) - return NULL; + goto out; /* * If we got a NULL ctid from the subplan, the @@ -3201,6 +3238,15 @@ lmerge_matched: * we need to switch to the NOT MATCHED BY * SOURCE case. */ + if (resultRelInfo->ri_needLockTagTuple) + { + if (ItemPointerIsValid(&lockedtid)) + UnlockTuple(resultRelInfo->ri_RelationDesc, &lockedtid, + InplaceUpdateTupleLock); + LockTuple(resultRelInfo->ri_RelationDesc, &context->tmfd.ctid, + InplaceUpdateTupleLock); + lockedtid = context->tmfd.ctid; + } if (!table_tuple_fetch_row_version(resultRelationDesc, &context->tmfd.ctid, SnapshotAny, @@ -3229,7 +3275,7 @@ lmerge_matched: * MATCHED [BY TARGET] actions */ *matched = false; - return NULL; + goto out; case TM_SelfModified: @@ -3257,13 +3303,13 @@ lmerge_matched: /* This shouldn't happen */ elog(ERROR, "attempted to update or delete invisible tuple"); - return NULL; + goto out; default: /* see table_tuple_lock call in ExecDelete() */ elog(ERROR, "unexpected table_tuple_lock status: %u", result); - return NULL; + goto out; } } @@ -3310,6 +3356,10 @@ lmerge_matched: /* * Successfully executed an action or no qualifying action was found. */ +out: + if (ItemPointerIsValid(&lockedtid)) + UnlockTuple(resultRelInfo->ri_RelationDesc, &lockedtid, + InplaceUpdateTupleLock); return rslot; } @@ -3761,6 +3811,7 @@ ExecModifyTable(PlanState *pstate) HeapTupleData oldtupdata; HeapTuple oldtuple; ItemPointer tupleid; + bool tuplock; CHECK_FOR_INTERRUPTS(); @@ -4073,6 +4124,8 @@ ExecModifyTable(PlanState *pstate) break; case CMD_UPDATE: + tuplock = false; + /* Initialize projection info if first time for this table */ if (unlikely(!resultRelInfo->ri_projectNewInfoValid)) ExecInitUpdateProjection(node, resultRelInfo); @@ -4084,6 +4137,7 @@ ExecModifyTable(PlanState *pstate) oldSlot = resultRelInfo->ri_oldTupleSlot; if (oldtuple != NULL) { + Assert(!resultRelInfo->ri_needLockTagTuple); /* Use the wholerow junk attr as the old tuple. */ ExecForceStoreHeapTuple(oldtuple, oldSlot, false); } @@ -4092,6 +4146,11 @@ ExecModifyTable(PlanState *pstate) /* Fetch the most recent version of old tuple. */ Relation relation = resultRelInfo->ri_RelationDesc; + if (resultRelInfo->ri_needLockTagTuple) + { + LockTuple(relation, tupleid, InplaceUpdateTupleLock); + tuplock = true; + } if (!table_tuple_fetch_row_version(relation, tupleid, SnapshotAny, oldSlot)) @@ -4103,6 +4162,9 @@ ExecModifyTable(PlanState *pstate) /* Now apply the update. */ slot = ExecUpdate(&context, resultRelInfo, tupleid, oldtuple, slot, node->canSetTag); + if (tuplock) + UnlockTuple(resultRelInfo->ri_RelationDesc, tupleid, + InplaceUpdateTupleLock); break; case CMD_DELETE: diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 262c987..99dbb5b 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -3727,6 +3727,7 @@ RelationSetNewRelfilenumber(Relation relation, char persistence) { RelFileNumber newrelfilenumber; Relation pg_class; + ItemPointerData otid; HeapTuple tuple; Form_pg_class classform; MultiXactId minmulti = InvalidMultiXactId; @@ -3769,11 +3770,12 @@ RelationSetNewRelfilenumber(Relation relation, char persistence) */ pg_class = table_open(RelationRelationId, RowExclusiveLock); - tuple = SearchSysCacheCopy1(RELOID, - ObjectIdGetDatum(RelationGetRelid(relation))); + tuple = SearchSysCacheLockedCopy1(RELOID, + ObjectIdGetDatum(RelationGetRelid(relation))); if (!HeapTupleIsValid(tuple)) elog(ERROR, "could not find tuple for relation %u", RelationGetRelid(relation)); + otid = tuple->t_self; classform = (Form_pg_class) GETSTRUCT(tuple); /* @@ -3893,9 +3895,10 @@ RelationSetNewRelfilenumber(Relation relation, char persistence) classform->relminmxid = minmulti; classform->relpersistence = persistence; - CatalogTupleUpdate(pg_class, &tuple->t_self, tuple); + CatalogTupleUpdate(pg_class, &otid, tuple); } + UnlockTuple(pg_class, &otid, InplaceUpdateTupleLock); heap_freetuple(tuple); table_close(pg_class, RowExclusiveLock); diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c index 3e03dfc..50c9440 100644 --- a/src/backend/utils/cache/syscache.c +++ b/src/backend/utils/cache/syscache.c @@ -30,7 +30,10 @@ #include "catalog/pg_shseclabel_d.h" #include "common/int.h" #include "lib/qunique.h" +#include "miscadmin.h" +#include "storage/lmgr.h" #include "utils/catcache.h" +#include "utils/inval.h" #include "utils/lsyscache.h" #include "utils/rel.h" #include "utils/syscache.h" @@ -269,6 +272,98 @@ ReleaseSysCache(HeapTuple tuple) } /* + * SearchSysCacheLocked1 + * + * Combine SearchSysCache1() with acquiring a LOCKTAG_TUPLE at mode + * InplaceUpdateTupleLock. This is a tool for complying with the + * README.tuplock section "Locking to write inplace-updated tables". After + * the caller's heap_update(), it should UnlockTuple(InplaceUpdateTupleLock) + * and ReleaseSysCache(). + * + * The returned tuple may be the subject of an uncommitted update, so this + * doesn't prevent the "tuple concurrently updated" error. + */ +HeapTuple +SearchSysCacheLocked1(int cacheId, + Datum key1) +{ + ItemPointerData tid; + LOCKTAG tag; + Oid dboid = + SysCache[cacheId]->cc_relisshared ? InvalidOid : MyDatabaseId; + Oid reloid = cacheinfo[cacheId].reloid; + + /*---------- + * Since inplace updates may happen just before our LockTuple(), we must + * return content acquired after LockTuple() of the TID we return. If we + * just fetched twice instead of looping, the following sequence would + * defeat our locking: + * + * GRANT: SearchSysCache1() = TID (1,5) + * GRANT: LockTuple(pg_class, (1,5)) + * [no more inplace update of (1,5) until we release the lock] + * CLUSTER: SearchSysCache1() = TID (1,5) + * CLUSTER: heap_update() = TID (1,8) + * CLUSTER: COMMIT + * GRANT: SearchSysCache1() = TID (1,8) + * GRANT: return (1,8) from SearchSysCacheLocked1() + * VACUUM: SearchSysCache1() = TID (1,8) + * VACUUM: LockTuple(pg_class, (1,8)) # two TIDs now locked for one rel + * VACUUM: inplace update + * GRANT: heap_update() = (1,9) # lose inplace update + * + * In the happy case, this takes two fetches, one to determine the TID to + * lock and another to get the content and confirm the TID didn't change. + * + * This is valid even if the row gets updated to a new TID, the old TID + * becomes LP_UNUSED, and the row gets updated back to its old TID. We'd + * still hold the right LOCKTAG_TUPLE and a copy of the row captured after + * the LOCKTAG_TUPLE. + */ + ItemPointerSetInvalid(&tid); + for (;;) + { + HeapTuple tuple; + LOCKMODE lockmode = InplaceUpdateTupleLock; + + tuple = SearchSysCache1(cacheId, key1); + if (ItemPointerIsValid(&tid)) + { + if (!HeapTupleIsValid(tuple)) + { + LockRelease(&tag, lockmode, false); + return tuple; + } + if (ItemPointerEquals(&tid, &tuple->t_self)) + return tuple; + LockRelease(&tag, lockmode, false); + } + else if (!HeapTupleIsValid(tuple)) + return tuple; + + tid = tuple->t_self; + ReleaseSysCache(tuple); + /* like: LockTuple(rel, &tid, lockmode) */ + SET_LOCKTAG_TUPLE(tag, dboid, reloid, + ItemPointerGetBlockNumber(&tid), + ItemPointerGetOffsetNumber(&tid)); + (void) LockAcquire(&tag, lockmode, false, false); + + /* + * If an inplace update just finished, ensure we process the syscache + * inval. XXX this is insufficient: the inplace updater may not yet + * have reached AtEOXact_Inval(). See test at inplace-inval.spec. + * + * If a heap_update() call just released its LOCKTAG_TUPLE, we'll + * probably find the old tuple and reach "tuple concurrently updated". + * If that heap_update() aborts, our LOCKTAG_TUPLE blocks inplace + * updates while our caller works. + */ + AcceptInvalidationMessages(); + } +} + +/* * SearchSysCacheCopy * * A convenience routine that does SearchSysCache and (if successful) @@ -295,6 +390,28 @@ SearchSysCacheCopy(int cacheId, } /* + * SearchSysCacheLockedCopy1 + * + * Meld SearchSysCacheLockedCopy1 with SearchSysCacheCopy(). After the + * caller's heap_update(), it should UnlockTuple(InplaceUpdateTupleLock) and + * heap_freetuple(). + */ +HeapTuple +SearchSysCacheLockedCopy1(int cacheId, + Datum key1) +{ + HeapTuple tuple, + newtuple; + + tuple = SearchSysCacheLocked1(cacheId, key1); + if (!HeapTupleIsValid(tuple)) + return tuple; + newtuple = heap_copytuple(tuple); + ReleaseSysCache(tuple); + return newtuple; +} + +/* * SearchSysCacheExists * * A convenience routine that just probes to see if a tuple can be found. diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 8bc421e..abd68e2 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -482,6 +482,9 @@ typedef struct ResultRelInfo /* Have the projection and the slots above been initialized? */ bool ri_projectNewInfoValid; + /* updates do LockTuple() before oldtup read; see README.tuplock */ + bool ri_needLockTagTuple; + /* triggers to be fired, if any */ TriggerDesc *ri_TrigDesc; diff --git a/src/include/storage/lockdefs.h b/src/include/storage/lockdefs.h index 934ba84..810b297 100644 --- a/src/include/storage/lockdefs.h +++ b/src/include/storage/lockdefs.h @@ -47,6 +47,8 @@ typedef int LOCKMODE; #define MaxLockMode 8 /* highest standard lock mode */ +/* See README.tuplock section "Locking to write inplace-updated tables" */ +#define InplaceUpdateTupleLock ExclusiveLock /* WAL representation of an AccessExclusiveLock on a table */ typedef struct xl_standby_lock diff --git a/src/include/utils/syscache.h b/src/include/utils/syscache.h index 03a27dd..b541911 100644 --- a/src/include/utils/syscache.h +++ b/src/include/utils/syscache.h @@ -43,9 +43,14 @@ extern HeapTuple SearchSysCache4(int cacheId, extern void ReleaseSysCache(HeapTuple tuple); +extern HeapTuple SearchSysCacheLocked1(int cacheId, + Datum key1); + /* convenience routines */ extern HeapTuple SearchSysCacheCopy(int cacheId, Datum key1, Datum key2, Datum key3, Datum key4); +extern HeapTuple SearchSysCacheLockedCopy1(int cacheId, + Datum key1); extern bool SearchSysCacheExists(int cacheId, Datum key1, Datum key2, Datum key3, Datum key4); extern Oid GetSysCacheOid(int cacheId, AttrNumber oidcol, diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out index c2a9841..b5fe8b0 100644 --- a/src/test/isolation/expected/intra-grant-inplace.out +++ b/src/test/isolation/expected/intra-grant-inplace.out @@ -154,9 +154,11 @@ step b1: BEGIN; step grant1: GRANT SELECT ON intra_grant_inplace TO PUBLIC; -step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); -step c2: COMMIT; +step addk2: ALTER TABLE intra_grant_inplace ADD PRIMARY KEY (c); +step addk2: <... completed> +ERROR: deadlock detected step grant1: <... completed> +step c2: COMMIT; step c1: COMMIT; step read2: SELECT relhasindex FROM pg_class @@ -194,9 +196,8 @@ relhasindex f (1 row) -s4: WARNING: got: tuple concurrently updated -step revoke4: <... completed> step r3: ROLLBACK; +step revoke4: <... completed> starting permutation: b1 drop1 b3 sfu3 revoke4 c1 r3 step b1: BEGIN; @@ -223,6 +224,6 @@ relhasindex ----------- (0 rows) -s4: WARNING: got: tuple concurrently deleted +s4: WARNING: got: cache lookup failed for relation REDACTED step revoke4: <... completed> step r3: ROLLBACK; diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index 3a74406..07307e6 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -194,7 +194,7 @@ step simplepartupdate_noroute { update parttbl set b = 2 where c = 1 returning *; } -# test system class updates +# test system class LockTuple() step sys1 { UPDATE pg_class SET reltuples = 123 WHERE oid = 'accounts'::regclass; diff --git a/src/test/isolation/specs/intra-grant-inplace.spec b/src/test/isolation/specs/intra-grant-inplace.spec index eed0b52..2992c85 100644 --- a/src/test/isolation/specs/intra-grant-inplace.spec +++ b/src/test/isolation/specs/intra-grant-inplace.spec @@ -14,6 +14,7 @@ teardown # heap_update() session s1 +setup { SET deadlock_timeout = '100s'; } step b1 { BEGIN; } step grant1 { GRANT SELECT ON intra_grant_inplace TO PUBLIC; @@ -25,6 +26,7 @@ step c1 { COMMIT; } # inplace update session s2 +setup { SET deadlock_timeout = '10ms'; } step read2 { SELECT relhasindex FROM pg_class WHERE oid = 'intra_grant_inplace'::regclass; @@ -73,8 +75,6 @@ step keyshr5 { teardown { ROLLBACK; } -# XXX extant bugs: permutation comments refer to planned future LockTuple() - permutation b1 grant1 @@ -126,8 +126,8 @@ permutation b2 sfnku2 b1 - grant1(c2) # acquire LockTuple(), await sfnku2 xmax - addk2 # block in LockTuple() behind grant1 = deadlock + grant1(addk2) # acquire LockTuple(), await sfnku2 xmax + addk2(*) # block in LockTuple() behind grant1 = deadlock c2 c1 read2 @@ -138,7 +138,7 @@ permutation grant1 b3 sfu3(c1) # acquire LockTuple(), await grant1 xmax - revoke4(sfu3) # block in LockTuple() behind sfu3 + revoke4(r3) # block in LockTuple() behind sfu3 c1 r3 # revoke4 unlocks old tuple and finds new --3pz3+3yBgJC2Q0yV--