Recheck checksum state before file_copy during CREATE DATABASE

The file_copy strategy check in createdb() runs during option
validation, before the transaction has an XID and before the
pg_database row exists, so the datachecksumsworker launcher
can start in that window and see neither the new database nor
the transaction creating it.  It then raw-copies a template
that was not processed yet, and those files stay unchecksummed,
failing verification from then on.

Recheck the state in CreateDatabaseUsingFileCopy(): the XID is
assigned by then, so a launcher starting after this point waits
for the transaction and finds the new database, and the copy
errors out instead. Add an injection point before the catalog
insert to test the window.

Backpatch to v19 where online checksums were introduced.

Author: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAN4CZFPEBsz8JeY4ixQ1V4ZL_xOY6pJaZS8ZLGH7R+wF--pEtg@mail.gmail.com
Backpatch-through: 19

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 51dcbd9cace4f84b31aaa3ddee068974cddf963c..e9766bcd7efc10ca3ef9559a923a499c454f0126 100644 (file)
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -65,6 +65,7 @@
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/injection_point.h"
 #include "utils/lsyscache.h"
 #include "utils/pg_locale.h"
 #include "utils/relmapper.h"
@@ -558,6 +559,23 @@ CreateDatabaseUsingFileCopy(Oid src_dboid, Oid dst_dboid, Oid src_tsid,
        Relation        rel;
        HeapTuple       tuple;
 
+       /*
+        * The strategy check in createdb() runs before our transaction has an XID
+        * and before the pg_database row exists, so the datachecksumsworker
+        * launcher can start in that window and miss both the new database and
+        * our transaction, leaving the raw-copied files without checksums.
+        */
+       if (DataChecksumsInProgressOn())
+               ereport(ERROR,
+                               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                               errmsg("create database strategy \"%s\" not allowed when data checksums are being enabled",
+                                          "file_copy"));
+
+       /*
+        * The XID is assigned by now, so a datachecksumsworker launcher starting
+        * after this point will wait for us and find the new database.
+        */
+
        /*
         * Force a checkpoint before starting the copy. This will force all dirty
         * buffers, including those of unlogged tables, out to disk, to ensure
@@ -1045,6 +1063,14 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
                        dbstrategy = CREATEDB_WAL_LOG;
                else if (pg_strcasecmp(strategy, "file_copy") == 0)
                {
+                       /*
+                        * If data checksums are being enabled we must not use file_copy
+                        * since it might copy source database which hasn't yet had data
+                        * checksums enabled, and the destination database will be skipped
+                        * as it's expected to have data checksums enabled.  Once we have
+                        * an XID assigned this needs to be rechecked, but if can error
+                        * out already we can save a lot of work.
+                        */
                        if (DataChecksumsInProgressOn())
                                ereport(ERROR,
                                                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -1510,6 +1536,8 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
        tuple = heap_form_tuple(RelationGetDescr(pg_database_rel),
                                                        new_record, new_record_nulls);
 
+       INJECTION_POINT("createdb-before-catalog-insert", NULL);
+
        CatalogTupleInsert(pg_database_rel, tuple);
 
        /*

diff --git a/src/test/modules/test_checksums/t/005_injection.pl b/src/test/modules/test_checksums/t/005_injection.pl
index 7240b93bdd135a9e61be0be5811cd2031e453166..34cd47e6c81be2db0daaa2f679e5520849f8d214 100644 (file)
--- a/src/test/modules/test_checksums/t/005_injection.pl
+++ b/src/test/modules/test_checksums/t/005_injection.pl
@@ -76,5 +76,73 @@ SKIP:
        enable_data_checksums($node, wait => 'on');
 }
 
+# ---------------------------------------------------------------------------
+# Test concurrent CREATE DATABASE which use the file_copy strategy
+#
+
+disable_data_checksums($node, wait => 1);
+my $node_loglocation = -s $node->logfile;
+
+$node->safe_psql('postgres',
+       "CREATE TABLE t AS SELECT generate_series(1,10000) AS a;");
+
+$node->safe_psql('postgres',
+       "SELECT injection_points_attach('createdb-before-catalog-insert','wait');"
+);
+$node->safe_psql('postgres',
+       "SELECT injection_points_attach('datachecksumsworker-fake-temptable-wait','wait');"
+);
+
+# Hold CREATE DATABASE after the strategy check, before its xact is visible.
+my $bg = $node->background_psql('postgres');
+$bg->query_until(
+       qr/starting_create/, q(
+\echo starting_create
+CREATE DATABASE fcdb TEMPLATE template0 STRATEGY file_copy;
+));
+$node->wait_for_event('client backend', 'createdb-before-catalog-insert');
+
+# Enable checksums, worker holds before processing template0.
+enable_data_checksums($node);
+$node->wait_for_event('datachecksums worker',
+       'datachecksumsworker-fake-temptable-wait');
+
+# Release CREATE DATABASE, must fail on the recheck instead of raw-copying.
+$node->safe_psql('postgres',
+       "SELECT injection_points_wakeup('createdb-before-catalog-insert');");
+$node->safe_psql('postgres',
+       "SELECT injection_points_detach('createdb-before-catalog-insert');");
+
+# Wait for the CREATE DATABASE xact to finish before releasing the worker.
+$node->poll_query_until('postgres',
+               "SELECT count(*) = 0 FROM pg_catalog.pg_stat_activity "
+         . "WHERE query LIKE 'CREATE DATABASE%' AND state != 'idle';");
+
+$node->safe_psql('postgres',
+       "SELECT injection_points_wakeup('datachecksumsworker-fake-temptable-wait');"
+);
+$node->safe_psql('postgres',
+       "SELECT injection_points_detach('datachecksumsworker-fake-temptable-wait');"
+);
+
+wait_for_checksum_state($node, 'on');
+
+my $result = $node->safe_psql('postgres',
+       "SELECT count(*) FROM pg_catalog.pg_database WHERE datname = 'fcdb';");
+is($result, '0', 'file_copy database creation was refused');
+
+my $log =
+  PostgreSQL::Test::Utils::slurp_file($node->logfile, $node_loglocation);
+like(
+       $log,
+       qr/create database strategy "file_copy" not allowed/m,
+       'file_copy error message in log');
+
+# ---------------------------------------------------------------------------
+# Test teardown
+#
+
+$bg->{run}->finish;
+$bg->quit;
 $node->stop;
 done_testing();