Add checks in pg_rewind to abort if backup_label file is present
Hi,
Please find the attached patch for $subject and associated test. Please review.
--
Thanks and Regards,
Krishnakumar (KK).
[Microsoft]
From 80ad7293b57a2b346b0775b8f6e0e06198617154 Mon Sep 17 00:00:00 2001
From: "Krishnakumar R (KK)"
Date: Tue, 5 Dec 2023 02:36:32 -0800
Subject: [PATCH v1] Add checks in pg_rewind to abort if backup_label file is
found in either source or target cluster. Append rewind tap tests to verify
the check.
---
src/bin/pg_rewind/file_ops.c | 17 +
src/bin/pg_rewind/file_ops.h | 2 +-
src/bin/pg_rewind/libpq_source.c | 22
src/bin/pg_rewind/local_source.c | 14
src/bin/pg_rewind/pg_rewind.c| 6 +++-
src/bin/pg_rewind/rewind_source.h| 5 +++
src/bin/pg_rewind/t/001_basic.pl | 46 ++--
src/test/perl/PostgreSQL/Test/Cluster.pm | 31
8 files changed, 138 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c
index dd22685932..d4568e7d09 100644
--- a/src/bin/pg_rewind/file_ops.c
+++ b/src/bin/pg_rewind/file_ops.c
@@ -299,6 +299,23 @@ sync_target_dir(void)
sync_pgdata(datadir_target, PG_VERSION_NUM, sync_method);
}
+/*
+ * Check if a file is present using the stat function. Return 'true'
+ * if present and 'false' if not.
+ */
+bool
+is_file_present(const char *datadir, const char *path)
+{
+ struct stat s;
+ char fullpath[MAXPGPATH];
+
+ snprintf(fullpath, sizeof(fullpath), "%s/%s", datadir, path);
+
+ if (stat(fullpath, ) < 0)
+ return false;
+
+ return true;
+}
/*
* Read a file into memory. The file to be read is /.
diff --git a/src/bin/pg_rewind/file_ops.h b/src/bin/pg_rewind/file_ops.h
index 427cf8e0b5..c063175fac 100644
--- a/src/bin/pg_rewind/file_ops.h
+++ b/src/bin/pg_rewind/file_ops.h
@@ -20,7 +20,7 @@ extern void truncate_target_file(const char *path, off_t newsize);
extern void create_target(file_entry_t *entry);
extern void remove_target(file_entry_t *entry);
extern void sync_target_dir(void);
-
+extern bool is_file_present(const char *datadir, const char *path);
extern char *slurpFile(const char *datadir, const char *path, size_t *filesize);
typedef void (*process_file_callback_t) (const char *path, file_type_t type, size_t size, const char *link_target);
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 417c74cfef..66ed6e156d 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -61,6 +61,7 @@ static void appendArrayEscapedString(StringInfo buf, const char *str);
static void process_queued_fetch_requests(libpq_source *src);
/* public interface functions */
+static bool libpq_is_file_present(rewind_source *source, const char *path);
static void libpq_traverse_files(rewind_source *source,
process_file_callback_t callback);
static void libpq_queue_fetch_file(rewind_source *source, const char *path, size_t len);
@@ -87,6 +88,7 @@ init_libpq_source(PGconn *conn)
src = pg_malloc0(sizeof(libpq_source));
+ src->common.is_file_present = libpq_is_file_present;
src->common.traverse_files = libpq_traverse_files;
src->common.fetch_file = libpq_fetch_file;
src->common.queue_fetch_file = libpq_queue_fetch_file;
@@ -627,6 +629,26 @@ appendArrayEscapedString(StringInfo buf, const char *str)
appendStringInfoCharMacro(buf, '\"');
}
+/*
+ * Check if a file is present using the connection to the
+ * database.
+ */
+static bool
+libpq_is_file_present(rewind_source *source, const char *path)
+{
+ PGconn *conn = ((libpq_source *) source)->conn;
+ PGresult *res;
+ const char *paramValues[1];
+
+ paramValues[0] = path;
+ res = PQexecParams(conn, "SELECT pg_stat_file($1)",
+ 1, NULL, paramValues, NULL, NULL, 1);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ return false;
+
+ return true;
+}
+
/*
* Fetch a single file as a malloc'd buffer.
*/
diff --git a/src/bin/pg_rewind/local_source.c b/src/bin/pg_rewind/local_source.c
index 9bd43cba74..0d6bf403b4 100644
--- a/src/bin/pg_rewind/local_source.c
+++ b/src/bin/pg_rewind/local_source.c
@@ -25,6 +25,7 @@ typedef struct
const char *datadir; /* path to the source data directory */
} local_source;
+static bool local_is_file_present(rewind_source *source, const char *path);
static void local_traverse_files(rewind_source *source,
process_file_callback_t callback);
static char *local_fetch_file(rewind_source *source, const char *path,
@@ -43,6 +44,7 @@ init_local_source(const char *datadir)
src = pg_malloc0(sizeof(local_source));
+ src->common.is_file_present = local_is_file_present;
src->common.traverse_files = local_traverse_files;
src->common.fetch_file = local_fetch_file;
src->common.queue_fetch_file = local_queue_fetch_file;
@@ -62,6 +64,18 @@ local_traverse_files(rewind_source *source, process_file_callback_t callback
Move bki file pre-processing from initdb - part 1 - initdb->genbki.pl
Hi,
As per discussion in [1] splitting the patch. Part1 moves replacement
logic in initdb of NAMEDATALEN, FLOAT8PASSBYVAL, SIZEOF_VOID_P,
ALIGNOF_POINTER to compile time via genbki.pl.
--
Thanks and regards,
Krishnakumar (KK).
[Microsoft]
[1]
https://www.postgresql.org/message-id/flat/CAPMWgZ9TCByVjpfdsgyte4rx%3DYsrAttYay2xDK4UN4Lm%3D-wJMQ%40mail.gmail.com
From 2b968de3dd559aa31095214f96773569ebec8b21 Mon Sep 17 00:00:00 2001
From: "Krishnakumar R (KK)"
Date: Mon, 4 Dec 2023 01:25:20 -0800
Subject: [PATCH v4] Move some BKI token replacement from initdb to compile
time via genbki.pl.
Here are some details:
- NAMEDATALEN, FLOAT8PASSBYVAL, SIZEOF_VOID_P, ALIGNOF_POINTER are replaced
during compilation from genbki.pl by reading header files.
- SIZEOF_VOID_P is available only after configuration (in pg_config.h).
A new parameter include-conf had to be added to genbki to point to header files
generated after configuration.
---
src/backend/catalog/Makefile| 2 +-
src/backend/catalog/genbki.pl | 34 -
src/bin/initdb/initdb.c | 12
src/include/catalog/meson.build | 1 +
src/tools/msvc/Solution.pm | 2 +-
5 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index ec7b6f5362..9859b52662 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -167,7 +167,7 @@ generated-header-symlinks: $(top_builddir)/src/include/catalog/header-stamp
# instead is cheating a bit, but it will achieve the goal of updating the
# version number when it changes.
bki-stamp: genbki.pl Catalog.pm $(POSTGRES_BKI_SRCS) $(POSTGRES_BKI_DATA) $(top_srcdir)/configure.ac $(top_srcdir)/src/include/access/transam.h
- $(PERL) $< --include-path=$(top_srcdir)/src/include/ \
+ $(PERL) $< --include-path=$(top_srcdir)/src/include/ --include-conf=$(top_builddir)/src/include/ \
--set-version=$(MAJORVERSION) $(POSTGRES_BKI_SRCS)
touch $@
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 380bc23c82..f7c8390e6d 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -25,13 +25,15 @@ use Catalog;
my $output_path = '';
my $major_version;
my $include_path;
+my $include_conf;
my $num_errors = 0;
GetOptions(
'output:s' => \$output_path,
'set-version:s' => \$major_version,
- 'include-path:s' => \$include_path) || usage();
+ 'include-path:s' => \$include_path,
+ 'include-conf:s' => \$include_conf) || usage();
# Sanity check arguments.
die "No input files.\n" unless @ARGV;
@@ -39,6 +41,7 @@ die "--set-version must be specified.\n" unless $major_version;
die "Invalid version string: $major_version\n"
unless $major_version =~ /^\d+$/;
die "--include-path must be specified.\n" unless $include_path;
+die "--include-conf must be specified.\n" unless $include_conf;
# Make sure paths end with a slash.
if ($output_path ne '' && substr($output_path, -1) ne '/')
@@ -180,6 +183,12 @@ my $FirstUnpinnedObjectId =
# Hash of next available OID, indexed by catalog name.
my %GenbkiNextOids;
+my $NameDataLen=Catalog::FindDefinedSymbol('pg_config_manual.h', $include_path,
+ 'NAMEDATALEN');
+my $SizeOfPointer=Catalog::FindDefinedSymbol('pg_config.h', $include_conf,
+ 'SIZEOF_VOID_P');
+my $Float8PassByVal=$SizeOfPointer >= 8 ? "true": "false";
+my $AlignOfPointer=$SizeOfPointer == 4 ? "i" : "d";
# Fetch some special data that we will substitute into the output file.
# CAUTION: be wary about what symbols you substitute into the .bki file here!
@@ -634,6 +643,23 @@ EOM
my $symbol = form_pg_type_symbol($bki_values{typname});
$bki_values{oid_symbol} = $symbol
if defined $symbol;
+
+ if ($bki_values{typlen} eq 'NAMEDATALEN')
+ {
+$bki_values{typlen} = $NameDataLen;
+ }
+ if ($bki_values{typlen} eq 'SIZEOF_POINTER')
+ {
+$bki_values{typlen} = $SizeOfPointer;
+ }
+ if ($bki_values{typalign} eq 'ALIGNOF_POINTER')
+ {
+$bki_values{typalign} = $AlignOfPointer;
+ }
+ if ($bki_values{typbyval} eq 'FLOAT8PASSBYVAL')
+ {
+$bki_values{typbyval} = $Float8PassByVal;
+ }
}
# Write to postgres.bki
@@ -812,6 +838,11 @@ sub gen_pg_attribute
($row{attnotnull} eq 't'
&& ($row{attlen} eq 'NAMEDATALEN' || $row{attlen} > 0));
+ if ($row{attnotnull} eq 't' && ($row{attlen} eq 'NAMEDATALEN'))
+ {
+$row{attlen} = $NameDataLen;
+ }
+
# If it's bootstrapped, put an entry in postgres.bki.
print_bki_insert(\%row, $schema) if $table->{bootstrap};
@@ -1106,6 +1137,7 @@ Options:
--output Output directory (default '.')
--set-versionPostgreSQL version number for initdb cross-check
--include-path Include path in source tree
+--include-conf Include file path generated
Re: Add missing error codes to PANIC/FATAL error reports in xlogrecovery
Hi,
Updated the patch with ERRCODE_CLUSTER_CORRUPTED & kept
ERRCODE_DATA_CORRUPTED when recovery is not consistent.
> > > Hm, this one arguably is not corruption, but we still cannot
> > > continue. ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE or maybe a new error
> > > code?
Added a ERRCODE_TIMELINE_INCONSISTENT to be specific about the
scenarios with timeline mismatches. Thoughts ?
>> Another aside: Isn't the hint here obsolete since we've removed exclusive
backups? I can't think of any scenario now where removing backup_label would
be correct in a non-exclusive backup.
Attached another patch which applies on top of the first patch to
remove the obsolete hint.
- KK
From b779b53ee0cde0ab239c44f5c6c83ec530c194ab Mon Sep 17 00:00:00 2001
From: "Krishnakumar R (KK)"
Date: Thu, 30 Nov 2023 00:56:40 -0800
Subject: [PATCH v2 1/2] Add missing error codes to PANIC/FATAL error reports.
---
src/backend/access/transam/xlogrecovery.c | 45 +++
src/backend/utils/errcodes.txt| 2 +
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c6156a..cb54f21de2 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -630,7 +630,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
if (!ReadRecord(xlogprefetcher, LOG, false,
checkPoint.ThisTimeLineID))
ereport(FATAL,
- (errmsg("could not find redo location referenced by checkpoint record"),
+ (errcode(ERRCODE_CLUSTER_CORRUPTED),
+ errmsg("could not find redo location referenced by checkpoint record"),
errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.\n"
"If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring from a backup.",
@@ -640,7 +641,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
else
{
ereport(FATAL,
- (errmsg("could not locate required checkpoint record"),
+ (errcode(ERRCODE_CLUSTER_CORRUPTED),
+ errmsg("could not locate required checkpoint record"),
errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.\n"
"If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring from a backup.",
@@ -764,7 +766,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
* simplify processing around checkpoints.
*/
ereport(PANIC,
- (errmsg("could not locate a valid checkpoint record")));
+ (errcode(ERRCODE_CLUSTER_CORRUPTED),
+ errmsg("could not locate a valid checkpoint record")));
}
memcpy(, XLogRecGetData(xlogreader), sizeof(CheckPoint));
wasShutdown = ((record->xl_info & ~XLR_INFO_MASK) == XLOG_CHECKPOINT_SHUTDOWN);
@@ -817,7 +820,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
*/
switchpoint = tliSwitchPoint(ControlFile->checkPointCopy.ThisTimeLineID, expectedTLEs, NULL);
ereport(FATAL,
-(errmsg("requested timeline %u is not a child of this server's history",
+(errcode(ERRCODE_TIMELINE_INCONSISTENT),
+ errmsg("requested timeline %u is not a child of this server's history",
recoveryTargetTLI),
errdetail("Latest checkpoint is at %X/%X on timeline %u, but in the history of the requested timeline, the server forked off from that timeline at %X/%X.",
LSN_FORMAT_ARGS(ControlFile->checkPoint),
@@ -833,7 +837,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
tliOfPointInHistory(ControlFile->minRecoveryPoint - 1, expectedTLEs) !=
ControlFile->minRecoveryPointTLI)
ereport(FATAL,
-(errmsg("requested timeline %u does not contain minimum recovery point %X/%X on timeline %u",
+(errcode(ERRCODE_TIMELINE_INCONSISTENT),
+ errmsg("requested timeline %u does not contain minimum recovery point %X/%X on timeline %u",
recoveryTargetTLI,
LSN_FORMAT_ARGS(ControlFile->minRecoveryPoint),
ControlFile->minRecoveryPointTLI)));
@@ -861,12 +866,14 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
checkPoint.newestCommitTsXid)));
if (!TransactionIdIsNormal(XidFromFullTransactionId(checkPoint.nextXid)))
ereport(PANIC,
-(errmsg(&q
Add missing error codes to PANIC/FATAL error reports in xlogrecovery
Hi,
Please find a patch attached which adds missing sql error code in
error reports which are FATAL or PANIC, in xlogrecovery.
This will help with deducing patterns when looking at error reports
from multiple postgres instances.
--
Thanks and Regards,
Krishnakumar (KK).
[Microsoft]
From 4cc518f25710c512ba3f9452392dc6ea67c2248b Mon Sep 17 00:00:00 2001
From: "Krishnakumar R (KK)"
Date: Thu, 30 Nov 2023 00:56:40 -0800
Subject: [PATCH v1] Add missing error codes to PANIC/FATAL error reports.
---
src/backend/access/transam/xlogrecovery.c | 45 +++
1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index c6156a..2f50928e7e 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -630,7 +630,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
if (!ReadRecord(xlogprefetcher, LOG, false,
checkPoint.ThisTimeLineID))
ereport(FATAL,
- (errmsg("could not find redo location referenced by checkpoint record"),
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("could not find redo location referenced by checkpoint record"),
errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.\n"
"If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring from a backup.",
@@ -640,7 +641,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
else
{
ereport(FATAL,
- (errmsg("could not locate required checkpoint record"),
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("could not locate required checkpoint record"),
errhint("If you are restoring from a backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add required recovery options.\n"
"If you are not restoring from a backup, try removing the file \"%s/backup_label\".\n"
"Be careful: removing \"%s/backup_label\" will result in a corrupt cluster if restoring from a backup.",
@@ -764,7 +766,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
* simplify processing around checkpoints.
*/
ereport(PANIC,
- (errmsg("could not locate a valid checkpoint record")));
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("could not locate a valid checkpoint record")));
}
memcpy(, XLogRecGetData(xlogreader), sizeof(CheckPoint));
wasShutdown = ((record->xl_info & ~XLR_INFO_MASK) == XLOG_CHECKPOINT_SHUTDOWN);
@@ -817,7 +820,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
*/
switchpoint = tliSwitchPoint(ControlFile->checkPointCopy.ThisTimeLineID, expectedTLEs, NULL);
ereport(FATAL,
-(errmsg("requested timeline %u is not a child of this server's history",
+(errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("requested timeline %u is not a child of this server's history",
recoveryTargetTLI),
errdetail("Latest checkpoint is at %X/%X on timeline %u, but in the history of the requested timeline, the server forked off from that timeline at %X/%X.",
LSN_FORMAT_ARGS(ControlFile->checkPoint),
@@ -833,7 +837,8 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
tliOfPointInHistory(ControlFile->minRecoveryPoint - 1, expectedTLEs) !=
ControlFile->minRecoveryPointTLI)
ereport(FATAL,
-(errmsg("requested timeline %u does not contain minimum recovery point %X/%X on timeline %u",
+(errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("requested timeline %u does not contain minimum recovery point %X/%X on timeline %u",
recoveryTargetTLI,
LSN_FORMAT_ARGS(ControlFile->minRecoveryPoint),
ControlFile->minRecoveryPointTLI)));
@@ -861,12 +866,14 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
checkPoint.newestCommitTsXid)));
if (!TransactionIdIsNormal(XidFromFullTransactionId(checkPoint.nextXid)))
ereport(PANIC,
-(errmsg("invalid next transaction ID")));
+(errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("invalid next transaction ID")));
/* sanity check */
if (checkPoint.redo > CheckPointLoc)
ereport(PANIC,
-(errmsg("invalid redo in checkpoint record")));
+(errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("invalid redo in checkpoint record")));
/*
* Check whether we need to force recovery from WAL. If it appears to
@@ -877,7 +884,8 @@ InitWalRecovery(Contro
Re: Move bki file pre-processing from initdb to bootstrap
Thank you for review, Peter. Makes sense on the split part. Was starting to think in same lines, at the end of last iteration. Will come back shortly. On Fri, Nov 10, 2023 at 12:48 AM Peter Eisentraut wrote: > On 17.10.23 03:32, Krishnakumar R wrote: > >> The version comparison has been moved from initdb to bootstrap. This > >> created some compatibility problems with windows tests. For now I kept > >> the version check to not have \n added, which worked fine and serves > >> the purpose. However hoping to have something better in v3 in addition > >> to addressing any other comments. > > > > With help from Thomas, figured out that on windows fopen uses binary > > mode in the backend which causes issues with EOL. Please find the > > attached patch updated with a fix for this. > > I suggest that this patch set be split up into three incremental parts: > > 1. Move some build-time settings from initdb to postgres.bki. > 2. The database locale handling. > 3. The bki file handling. > > Each of these topics really needs a separate detailed consideration. > >
Re: Move bki file pre-processing from initdb to bootstrap
> The version comparison has been moved from initdb to bootstrap. This > created some compatibility problems with windows tests. For now I kept > the version check to not have \n added, which worked fine and serves > the purpose. However hoping to have something better in v3 in addition > to addressing any other comments. With help from Thomas, figured out that on windows fopen uses binary mode in the backend which causes issues with EOL. Please find the attached patch updated with a fix for this. -- Thanks and Regards, Krishnakumar (KK). [Microsoft] v3-0001-Remove-BKI-file-token-pre-processing-logic-from-i.patch Description: Binary data
Re: Move bki file pre-processing from initdb to bootstrap
Thank you, Peter, Andres and Tom for your comments and thoughts. Hi Peter, > For example, for NAMEDATALEN, SIZEOF_POINTER, ALIGNOF_POINTER, > FLOAT8PASSBYVAL, these are known at build time, so we could have > genbki.pl substitute them at build time. I have modified the patch to use genbki to generate these ones during build time. > The locale-related symbols (ENCODING, LC_COLLATE, etc.), I wonder > whether we can eliminate the need for them. Right now, these are only > used in the bki entry for the template1 database. How about initdb > creates template0 first, with hardcoded default encoding, collation, > etc., and then creates template1 from that, using the normal CREATE > DATABASE command with the appropriate options. Or initdb could just run > an UPDATE on pg_database to put the right settings in place. Using a combination of this suggestion and discussions Andres pointed to in this thread, updated the patch to add placeholder values first into template1 and then do UPDATEs in initdb itself. > You should use an option letter that isn't already in use in one of the > other modes of "postgres". We try to keep those consistent. > > New options should be added to the --help output (usage() in main.c). Used a -b option under bootstrap mode and added help. > elog(INFO, "Open bki file %s\n", bki_file); > + boot_yyin = fopen(bki_file, "r"); > > Why is this needed? It already reads the bki file from stdin? We no longer open the bki file in initdb and pass to postgres to parse from stdin, instead we open the bki file directly in bootstrap and pass the file stream to the parser. Hence the need to switch the yyin. Have added a comment in the commit logs to capture this. The version comparison has been moved from initdb to bootstrap. This created some compatibility problems with windows tests. For now I kept the version check to not have \n added, which worked fine and serves the purpose. However hoping to have something better in v3 in addition to addressing any other comments. Please let me know your thoughts and review comments. -- Thanks and Regards, Krishnakumar (KK). [Microsoft] On Tue, Sep 19, 2023 at 3:18 AM Peter Eisentraut wrote: > > On 01.09.23 10:01, Krishnakumar R wrote: > > This patch moves the pre-processing for tokens in the bki file from > > initdb to bootstrap. With these changes the bki file will only be > > opened once in bootstrap and parsing will be done by the bootstrap > > parser. > > I did some rough performance tests on this. I get about a 10% > improvement on initdb run time, so this appears to have merit. > > I wonder whether we can reduce the number of symbols that we need this > treatment for. > > For example, for NAMEDATALEN, SIZEOF_POINTER, ALIGNOF_POINTER, > FLOAT8PASSBYVAL, these are known at build time, so we could have > genbki.pl substitute them at build time. > > The locale-related symbols (ENCODING, LC_COLLATE, etc.), I wonder > whether we can eliminate the need for them. Right now, these are only > used in the bki entry for the template1 database. How about initdb > creates template0 first, with hardcoded default encoding, collation, > etc., and then creates template1 from that, using the normal CREATE > DATABASE command with the appropriate options. Or initdb could just run > an UPDATE on pg_database to put the right settings in place. > > I don't like this part so much, because it adds like 4 more places each > of these variables is mentioned, which increases the mental and testing > overhead for dealing with these features (which are an area of active > development). > > In general, it would be good if this could be factored a bit more so > each variable doesn't have to be hardcoded in so many places. > > > Some more detailed comments on the code: > > + boot_yylval.str = pstrdup(yytext); > + sprintf(boot_yylval.str, "%d", NAMEDATALEN); > > This is weird. You are first assigning the text and then overwriting it > with the numeric value? > > You can also use boot_yylval.ival for storing numbers. > > + if (bootp_null(ebootp, ebootp->username)) return > NULLVAL; > > Add proper line breaks in the code. > > +bool bootp_null(extra_bootstrap_params *e, char *s) > > Add a comment what this function is supposed to do. > > This function could be static. > > + while ((flag = getopt(argc, argv, "B:c:d:D:Fi:kr:X:-:")) != -1) > > You should use an option letter that isn't already in use in one of the > other modes of "postgres". We try to keep those consistent. > > New options should be added to the --help output (usage() in main.c). > > + elog(INFO, &
Fixup the variable name to indicate the WAL record reservation status.
Hi All, Please find a small patch to improve code readability by fixing up the variable name to indicate the WAL record reservation status. The insertion is done later in the code based on the reservation status. -- Thanks and Regards, Krishnakumar (KK). [Microsoft] v1-0001-Fixup-the-variable-name-to-indicate-the-wal-recor.patch Description: Binary data
Small patch modifying variable name to reflect the logic involved
Hi All, Please find a small patch to improve code readability by modifying variable name to reflect the logic involved - finding diff between end and start time of WAL sync. -- Thanks and Regards, Krishnakumar (KK) [Microsoft] v1-0001-Improve-code-readability-by-modifying-variable-na.patch Description: Binary data
Move bki file pre-processing from initdb to bootstrap
Hi All, This patch moves the pre-processing for tokens in the bki file from initdb to bootstrap. With these changes the bki file will only be opened once in bootstrap and parsing will be done by the bootstrap parser. The flow of bki file processing will be as follows: - In initdb gather the values used to replace the tokens in the bki file. - Pass these values into postgres bootstrap startup using '-i' option as key-value pairs. - In bootstrap open the bki file (the bki file name was received as a parameter). - During the parsing of the bki file, replace the tokens received as parameters with their values. Related discussion can be found here: https://www.postgresql.org/message-id/20220216021219.ygzrtb3hd5bn7olz%40alap3.anarazel.de Note: Currently the patch breaks on windows due to placement of extra quotes when passing parameters (Thanks to Thomas Munro for helping me find that). Will follow up with v2 fixing the windows issues on passing the parameters and format fixes. Please review and provide feedback. -- Thanks and Regards, Krishnakumar (KK). [Microsoft] v1-0001-Move-the-pre-processing-for-tokens-in-bki-file-fr.patch Description: Binary data
