agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH v48 3/3] Replace assuming a composite object on a scalar
30+ messages / 6 participants
[nested] [flat]
* [PATCH v48 3/3] Replace assuming a composite object on a scalar
@ 2021-01-20 15:53 Dmitrii Dolgov <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Dmitrii Dolgov @ 2021-01-20 15:53 UTC (permalink / raw)
For jsonb subscripting assignment it could happen that the provided path
assumes an object or an array at some level, but the source jsonb has a
scalar value there. Originally the update operation will be skipped and
no message will be shown that nothing happened. Return an error to
indicate such situations.
---
doc/src/sgml/json.sgml | 19 +++++++++++++++++++
src/backend/utils/adt/jsonfuncs.c | 27 +++++++++++++++++++++++++++
src/test/regress/expected/jsonb.out | 27 +++++++++++++++++++++++++++
src/test/regress/sql/jsonb.sql | 17 +++++++++++++++++
4 files changed, 90 insertions(+)
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 07bd19f974..deeb9e66e0 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -614,8 +614,23 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
The result of a subscripting expression is always of the jsonb data type.
</para>
+ <para>
+ <command>UPDATE</command> statements may use subscripting in the
+ <literal>SET</literal> clause to modify <type>jsonb</type> values. Object
+ values being traversed must exist as specified by the subscript path. For
+ instance, the path <literal>val['a']['b']['c']</literal> assumes that
+ <literal>val</literal>, <literal>val['a']</literal>, and <literal>val['a']['b']</literal>
+ are all objects in every record being updated (<literal>val['a']['b']</literal>
+ may or may not contain a field named <literal>c</literal>, as long as it's an
+ object). If any individual <literal>val</literal>, <literal>val['a']</literal>,
+ or <literal>val['a']['b']</literal> is a non-object such as a string, a number,
+ or <literal>NULL</literal>, an error is raised even if other values do conform.
+ Array values are not subject to this restriction, as detailed below.
+ </para>
+
<para>
An example of subscripting syntax:
+
<programlisting>
-- Extract object value by key
@@ -631,6 +646,10 @@ SELECT ('[1, "2", null]'::jsonb)[1];
-- value must be of the jsonb type as well
UPDATE table_name SET jsonb_field['key'] = '1';
+-- This will raise an error if any record's jsonb_field['a']['b'] is something
+-- other than an object. For example, the value {"a": 1} has no 'b' key.
+UPDATE table_name SET jsonb_field['a']['b']['c'] = '1';
+
-- Filter records using a WHERE clause with subscripting. Since the result of
-- subscripting is jsonb, the value we compare it against must also be jsonb.
-- The double quotes make "value" also a valid jsonb string.
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index f14f6c3191..9138b7950e 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -4926,6 +4926,20 @@ setPath(JsonbIterator **it, Datum *path_elems,
switch (r)
{
case WJB_BEGIN_ARRAY:
+ /*
+ * If instructed complain about attempts to replace whithin a raw
+ * scalar value. This happens even when current level is equal to
+ * path_len, because the last path key should also correspond to an
+ * object or an array, not raw scalar.
+ */
+ if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1) &&
+ v.val.array.rawScalar)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot replace existing key"),
+ errdetail("The path assumes key is a composite object, "
+ "but it is a scalar value.")));
+
(void) pushJsonbValue(st, r, NULL);
setPathArray(it, path_elems, path_nulls, path_len, st, level,
newval, v.val.array.nElems, op_type);
@@ -4943,6 +4957,19 @@ setPath(JsonbIterator **it, Datum *path_elems,
break;
case WJB_ELEM:
case WJB_VALUE:
+ /*
+ * If instructed complain about attempts to replace whithin a
+ * scalar value. This happens even when current level is equal to
+ * path_len, because the last path key should also correspond to an
+ * object or an array, not an element or value.
+ */
+ if ((op_type & JB_PATH_FILL_GAPS) && (level <= path_len - 1))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot replace existing key"),
+ errdetail("The path assumes key is a composite object, "
+ "but it is a scalar value.")));
+
res = pushJsonbValue(st, r, &v);
break;
default:
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index 5b5510c4fd..cf0ce0e44c 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -5134,6 +5134,33 @@ select * from test_jsonb_subscript;
1 | {"a": [null, {"c": [null, null, 1]}]}
(1 row)
+-- trying replace assuming a composite object, but it's an element or a value
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": 1}');
+update test_jsonb_subscript set test_json['a']['b'] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+update test_jsonb_subscript set test_json['a']['b']['c'] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+update test_jsonb_subscript set test_json['a'][0] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+update test_jsonb_subscript set test_json['a'][0]['c'] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+update test_jsonb_subscript set test_json['a'][0][0] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+-- trying replace assuming a composite object, but it's a raw scalar
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, 'null');
+update test_jsonb_subscript set test_json[0] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
+update test_jsonb_subscript set test_json[0][0] = '1';
+ERROR: cannot replace existing key
+DETAIL: The path assumes key is a composite object, but it is a scalar value.
-- jsonb to tsvector
select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
to_tsvector
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 0320db0ea4..1a9d21741f 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -1371,6 +1371,23 @@ insert into test_jsonb_subscript values (1, '{"a": []}');
update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1';
select * from test_jsonb_subscript;
+-- trying replace assuming a composite object, but it's an element or a value
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": 1}');
+update test_jsonb_subscript set test_json['a']['b'] = '1';
+update test_jsonb_subscript set test_json['a']['b']['c'] = '1';
+update test_jsonb_subscript set test_json['a'][0] = '1';
+update test_jsonb_subscript set test_json['a'][0]['c'] = '1';
+update test_jsonb_subscript set test_json['a'][0][0] = '1';
+
+-- trying replace assuming a composite object, but it's a raw scalar
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, 'null');
+update test_jsonb_subscript set test_json[0] = '1';
+update test_jsonb_subscript set test_json[0][0] = '1';
+
-- jsonb to tsvector
select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
--
2.21.0
--m4t3wpskod5t4yye--
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v2 1/5] Define PG_REPLSLOT_DIR
@ 2024-08-14 09:16 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-14 09:16 UTC (permalink / raw)
Replace most of the places where "pg_replslot" is used in .c files with a new
PG_REPLSLOT_DIR define. The places where it is not done is for consistency with
the existing PG_STAT_TMP_DIR define.
---
src/backend/backup/basebackup.c | 3 +-
.../replication/logical/reorderbuffer.c | 15 ++++----
src/backend/replication/slot.c | 38 +++++++++----------
src/backend/utils/adt/genfile.c | 5 ++-
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_rewind/filemap.c | 2 +-
src/include/replication/slot.h | 2 +
7 files changed, 36 insertions(+), 31 deletions(-)
24.0% src/backend/replication/logical/
59.8% src/backend/replication/
8.7% src/backend/utils/adt/
4.1% src/bin/
3.2% src/
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 01b35e26bd..de16afac74 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -36,6 +36,7 @@
#include "port.h"
#include "postmaster/syslogger.h"
#include "postmaster/walsummarizer.h"
+#include "replication/slot.h"
#include "replication/walsender.h"
#include "replication/walsender_private.h"
#include "storage/bufpage.h"
@@ -161,7 +162,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ PG_REPLSLOT_DIR,
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
PG_DYNSHMEM_DIR,
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..6f98115d1e 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4573,7 +4573,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
- sprintf(path, "pg_replslot/%s", slotname);
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, slotname);
/* we're only handling directories here, skip if it's not ours */
if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
@@ -4586,14 +4586,14 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
if (strncmp(spill_de->d_name, "xid", 3) == 0)
{
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
if (unlink(path) != 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ PG_REPLSLOT_DIR, path, slotname)));
}
}
FreeDir(spill_dir);
@@ -4612,7 +4612,8 @@ ReorderBufferSerializedPath(char *path, ReplicationSlot *slot, TransactionId xid
XLogSegNoOffsetToRecPtr(segno, 0, wal_segment_size, recptr);
- snprintf(path, MAXPGPATH, "pg_replslot/%s/xid-%u-lsn-%X-%X.spill",
+ snprintf(path, MAXPGPATH, "%s/%s/xid-%u-lsn-%X-%X.spill",
+ PG_REPLSLOT_DIR,
NameStr(MyReplicationSlot->data.name),
xid, LSN_FORMAT_ARGS(recptr));
}
@@ -4627,8 +4628,8 @@ StartupReorderBuffer(void)
DIR *logical_dir;
struct dirent *logical_de;
- logical_dir = AllocateDir("pg_replslot");
- while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
+ logical_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((logical_de = ReadDir(logical_dir, PG_REPLSLOT_DIR)) != NULL)
{
if (strcmp(logical_de->d_name, ".") == 0 ||
strcmp(logical_de->d_name, "..") == 0)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c290339af5..55e161f8c9 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -20,7 +20,7 @@
* on standbys (to support cascading setups). The requirement that slots be
* usable on standbys precludes storing them in the system catalogs.
*
- * Each replication slot gets its own directory inside the $PGDATA/pg_replslot
+ * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
* directory. Inside that directory the state file will contain the slot's
* own data. Additional data can be stored alongside that file if required.
* While the server is running, the state data is also cached in memory for
@@ -916,8 +916,8 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
LWLockAcquire(ReplicationSlotAllocationLock, LW_EXCLUSIVE);
/* Generate pathnames. */
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* Rename the slot directory on disk, so that we'll no longer recognize
@@ -938,7 +938,7 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
*/
START_CRIT_SECTION();
fsync_fname(tmppath, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
else
@@ -1016,7 +1016,7 @@ ReplicationSlotSave(void)
Assert(MyReplicationSlot != NULL);
- sprintf(path, "pg_replslot/%s", NameStr(MyReplicationSlot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(MyReplicationSlot->data.name));
SaveSlotToPath(MyReplicationSlot, path, ERROR);
}
@@ -1881,7 +1881,7 @@ CheckPointReplicationSlots(bool is_shutdown)
continue;
/* save the slot to disk, locking is handled in SaveSlotToPath() */
- sprintf(path, "pg_replslot/%s", NameStr(s->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(s->data.name));
/*
* Slot's data is not flushed each time the confirmed_flush LSN is
@@ -1922,17 +1922,17 @@ StartupReplicationSlots(void)
elog(DEBUG1, "starting up replication slots");
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
- char path[MAXPGPATH + 12];
+ char path[MAXPGPATH + sizeof(PG_REPLSLOT_DIR)];
PGFileType de_type;
if (strcmp(replication_de->d_name, ".") == 0 ||
strcmp(replication_de->d_name, "..") == 0)
continue;
- snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, replication_de->d_name);
de_type = get_dirent_type(path, replication_de, false, DEBUG1);
/* we're only creating directories here, skip if it's not our's */
@@ -1949,7 +1949,7 @@ StartupReplicationSlots(void)
path)));
continue;
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
continue;
}
@@ -1987,8 +1987,8 @@ CreateSlotOnDisk(ReplicationSlot *slot)
* takes out the lock, if we'd take the lock here, we'd deadlock.
*/
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* It's just barely possible that some previous effort to create or drop a
@@ -2027,7 +2027,7 @@ CreateSlotOnDisk(ReplicationSlot *slot)
START_CRIT_SECTION();
fsync_fname(path, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
@@ -2170,7 +2170,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
fsync_fname(path, false);
fsync_fname(dir, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
@@ -2195,8 +2195,8 @@ RestoreSlotFromDisk(const char *name)
{
ReplicationSlotOnDisk cp;
int i;
- char slotdir[MAXPGPATH + 12];
- char path[MAXPGPATH + 22];
+ char slotdir[MAXPGPATH + sizeof(PG_REPLSLOT_DIR)];
+ char path[MAXPGPATH + sizeof(PG_REPLSLOT_DIR) + 10];
int fd;
bool restored = false;
int readBytes;
@@ -2205,7 +2205,7 @@ RestoreSlotFromDisk(const char *name)
/* no need to lock here, no concurrent access allowed yet */
/* delete temp file if it exists */
- sprintf(slotdir, "pg_replslot/%s", name);
+ sprintf(slotdir, "%s/%s", PG_REPLSLOT_DIR, name);
sprintf(path, "%s/state.tmp", slotdir);
if (unlink(path) < 0 && errno != ENOENT)
ereport(PANIC,
@@ -2332,7 +2332,7 @@ RestoreSlotFromDisk(const char *name)
(errmsg("could not remove directory \"%s\"",
slotdir)));
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
return;
}
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 0d82557417..e645e6b85a 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -708,7 +708,7 @@ pg_ls_logicalmapdir(PG_FUNCTION_ARGS)
}
/*
- * Function to return the list of files in the pg_replslot/<replication_slot>
+ * Function to return the list of files in the PG_REPLSLOT_DIR/<replication_slot>
* directory.
*/
Datum
@@ -728,6 +728,7 @@ pg_ls_replslotdir(PG_FUNCTION_ARGS)
errmsg("replication slot \"%s\" does not exist",
slotname)));
- snprintf(path, sizeof(path), "pg_replslot/%s", slotname);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, slotname);
+
return pg_ls_dir_files(fcinfo, path, false);
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f00718a015..2ee55eced0 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -242,7 +242,7 @@ static const char *const subdirs[] = {
"pg_multixact/offsets",
"base",
"base/1",
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
"pg_tblspc",
"pg_stat",
"pg_stat_tmp",
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4458324c9d..00e644d988 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -97,7 +97,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
"pg_dynshmem", /* defined as PG_DYNSHMEM_DIR */
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index c2ee149fd6..9665faac1c 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -17,6 +17,8 @@
#include "storage/spin.h"
#include "replication/walreceiver.h"
+#define PG_REPLSLOT_DIR "pg_replslot"
+
/*
* Behaviour of replication slots, upon release or crash.
*
--
2.34.1
--2N+tbir8xF9qUTGa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v2-0002-Define-PG_LOGICAL_MAPPINGS_DIR.patch"
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v3 1/6] Define PG_REPLSLOT_DIR
@ 2024-08-14 09:16 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-14 09:16 UTC (permalink / raw)
Replace most of the places where "pg_replslot" is used in .c files with a new
PG_REPLSLOT_DIR define. The places where it is not done is for consistency with
the existing PG_STAT_TMP_DIR define.
---
src/backend/backup/basebackup.c | 3 +-
.../replication/logical/reorderbuffer.c | 17 ++++-----
src/backend/replication/slot.c | 36 +++++++++----------
src/backend/utils/adt/genfile.c | 3 +-
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_rewind/filemap.c | 2 +-
src/include/replication/slot.h | 2 ++
7 files changed, 35 insertions(+), 30 deletions(-)
27.5% src/backend/replication/logical/
60.8% src/backend/replication/
3.9% src/backend/utils/adt/
4.2% src/bin/
3.3% src/
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 01b35e26bd..de16afac74 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -36,6 +36,7 @@
#include "port.h"
#include "postmaster/syslogger.h"
#include "postmaster/walsummarizer.h"
+#include "replication/slot.h"
#include "replication/walsender.h"
#include "replication/walsender_private.h"
#include "storage/bufpage.h"
@@ -161,7 +162,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ PG_REPLSLOT_DIR,
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
PG_DYNSHMEM_DIR,
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..78166b6ab7 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4571,9 +4571,9 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
DIR *spill_dir;
struct dirent *spill_de;
struct stat statbuf;
- char path[MAXPGPATH * 2 + 12];
+ char path[MAXPGPATH * 2 + sizeof(PG_REPLSLOT_DIR)];
- sprintf(path, "pg_replslot/%s", slotname);
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, slotname);
/* we're only handling directories here, skip if it's not ours */
if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
@@ -4586,14 +4586,14 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
if (strncmp(spill_de->d_name, "xid", 3) == 0)
{
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
if (unlink(path) != 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ path, PG_REPLSLOT_DIR, slotname)));
}
}
FreeDir(spill_dir);
@@ -4612,7 +4612,8 @@ ReorderBufferSerializedPath(char *path, ReplicationSlot *slot, TransactionId xid
XLogSegNoOffsetToRecPtr(segno, 0, wal_segment_size, recptr);
- snprintf(path, MAXPGPATH, "pg_replslot/%s/xid-%u-lsn-%X-%X.spill",
+ snprintf(path, MAXPGPATH, "%s/%s/xid-%u-lsn-%X-%X.spill",
+ PG_REPLSLOT_DIR,
NameStr(MyReplicationSlot->data.name),
xid, LSN_FORMAT_ARGS(recptr));
}
@@ -4627,8 +4628,8 @@ StartupReorderBuffer(void)
DIR *logical_dir;
struct dirent *logical_de;
- logical_dir = AllocateDir("pg_replslot");
- while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
+ logical_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((logical_de = ReadDir(logical_dir, PG_REPLSLOT_DIR)) != NULL)
{
if (strcmp(logical_de->d_name, ".") == 0 ||
strcmp(logical_de->d_name, "..") == 0)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c290339af5..1299d92696 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -916,8 +916,8 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
LWLockAcquire(ReplicationSlotAllocationLock, LW_EXCLUSIVE);
/* Generate pathnames. */
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* Rename the slot directory on disk, so that we'll no longer recognize
@@ -938,7 +938,7 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
*/
START_CRIT_SECTION();
fsync_fname(tmppath, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
else
@@ -1016,7 +1016,7 @@ ReplicationSlotSave(void)
Assert(MyReplicationSlot != NULL);
- sprintf(path, "pg_replslot/%s", NameStr(MyReplicationSlot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(MyReplicationSlot->data.name));
SaveSlotToPath(MyReplicationSlot, path, ERROR);
}
@@ -1881,7 +1881,7 @@ CheckPointReplicationSlots(bool is_shutdown)
continue;
/* save the slot to disk, locking is handled in SaveSlotToPath() */
- sprintf(path, "pg_replslot/%s", NameStr(s->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(s->data.name));
/*
* Slot's data is not flushed each time the confirmed_flush LSN is
@@ -1922,17 +1922,17 @@ StartupReplicationSlots(void)
elog(DEBUG1, "starting up replication slots");
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
- char path[MAXPGPATH + 12];
+ char path[MAXPGPATH + sizeof(PG_REPLSLOT_DIR)];
PGFileType de_type;
if (strcmp(replication_de->d_name, ".") == 0 ||
strcmp(replication_de->d_name, "..") == 0)
continue;
- snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, replication_de->d_name);
de_type = get_dirent_type(path, replication_de, false, DEBUG1);
/* we're only creating directories here, skip if it's not our's */
@@ -1949,7 +1949,7 @@ StartupReplicationSlots(void)
path)));
continue;
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
continue;
}
@@ -1987,8 +1987,8 @@ CreateSlotOnDisk(ReplicationSlot *slot)
* takes out the lock, if we'd take the lock here, we'd deadlock.
*/
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* It's just barely possible that some previous effort to create or drop a
@@ -2027,7 +2027,7 @@ CreateSlotOnDisk(ReplicationSlot *slot)
START_CRIT_SECTION();
fsync_fname(path, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
@@ -2170,7 +2170,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
fsync_fname(path, false);
fsync_fname(dir, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
@@ -2195,8 +2195,8 @@ RestoreSlotFromDisk(const char *name)
{
ReplicationSlotOnDisk cp;
int i;
- char slotdir[MAXPGPATH + 12];
- char path[MAXPGPATH + 22];
+ char slotdir[MAXPGPATH + sizeof(PG_REPLSLOT_DIR)];
+ char path[MAXPGPATH + sizeof(PG_REPLSLOT_DIR) + 10];
int fd;
bool restored = false;
int readBytes;
@@ -2205,7 +2205,7 @@ RestoreSlotFromDisk(const char *name)
/* no need to lock here, no concurrent access allowed yet */
/* delete temp file if it exists */
- sprintf(slotdir, "pg_replslot/%s", name);
+ sprintf(slotdir, "%s/%s", PG_REPLSLOT_DIR, name);
sprintf(path, "%s/state.tmp", slotdir);
if (unlink(path) < 0 && errno != ENOENT)
ereport(PANIC,
@@ -2332,7 +2332,7 @@ RestoreSlotFromDisk(const char *name)
(errmsg("could not remove directory \"%s\"",
slotdir)));
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
return;
}
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 0d82557417..d95683d041 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -728,6 +728,7 @@ pg_ls_replslotdir(PG_FUNCTION_ARGS)
errmsg("replication slot \"%s\" does not exist",
slotname)));
- snprintf(path, sizeof(path), "pg_replslot/%s", slotname);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, slotname);
+
return pg_ls_dir_files(fcinfo, path, false);
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f00718a015..2ee55eced0 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -242,7 +242,7 @@ static const char *const subdirs[] = {
"pg_multixact/offsets",
"base",
"base/1",
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
"pg_tblspc",
"pg_stat",
"pg_stat_tmp",
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4458324c9d..00e644d988 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -97,7 +97,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
"pg_dynshmem", /* defined as PG_DYNSHMEM_DIR */
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index c2ee149fd6..9665faac1c 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -17,6 +17,8 @@
#include "storage/spin.h"
#include "replication/walreceiver.h"
+#define PG_REPLSLOT_DIR "pg_replslot"
+
/*
* Behaviour of replication slots, upon release or crash.
*
--
2.34.1
--Q6rxrhKDN6RKlQzQ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v3-0002-Define-REPLSLOT_DIR_ARGS.patch"
^ permalink raw reply [nested|flat] 30+ messages in thread
* [PATCH v1] Define PG_REPLSLOT_DIR
@ 2024-08-14 09:16 Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-14 09:16 UTC (permalink / raw)
Replace most of the places where "pg_replslot" is used in .c files with a new
PG_REPLSLOT_DIR define. The places where it is not done is for consistency with
the existing PG_STAT_TMP_DIR define.
---
src/backend/backup/basebackup.c | 3 +-
.../replication/logical/reorderbuffer.c | 15 +++++----
src/backend/replication/slot.c | 32 +++++++++----------
src/backend/utils/adt/genfile.c | 5 +--
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_rewind/filemap.c | 2 +-
src/include/replication/slot.h | 2 ++
7 files changed, 33 insertions(+), 28 deletions(-)
25.9% src/backend/replication/logical/
56.5% src/backend/replication/
9.4% src/backend/utils/adt/
4.4% src/bin/
3.5% src/
diff --git a/src/backend/backup/basebackup.c b/src/backend/backup/basebackup.c
index 01b35e26bd..de16afac74 100644
--- a/src/backend/backup/basebackup.c
+++ b/src/backend/backup/basebackup.c
@@ -36,6 +36,7 @@
#include "port.h"
#include "postmaster/syslogger.h"
#include "postmaster/walsummarizer.h"
+#include "replication/slot.h"
#include "replication/walsender.h"
#include "replication/walsender_private.h"
#include "storage/bufpage.h"
@@ -161,7 +162,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ PG_REPLSLOT_DIR,
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
PG_DYNSHMEM_DIR,
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 00a8327e77..6f98115d1e 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -4573,7 +4573,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
- sprintf(path, "pg_replslot/%s", slotname);
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, slotname);
/* we're only handling directories here, skip if it's not ours */
if (lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
@@ -4586,14 +4586,14 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
if (strncmp(spill_de->d_name, "xid", 3) == 0)
{
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
if (unlink(path) != 0)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ PG_REPLSLOT_DIR, path, slotname)));
}
}
FreeDir(spill_dir);
@@ -4612,7 +4612,8 @@ ReorderBufferSerializedPath(char *path, ReplicationSlot *slot, TransactionId xid
XLogSegNoOffsetToRecPtr(segno, 0, wal_segment_size, recptr);
- snprintf(path, MAXPGPATH, "pg_replslot/%s/xid-%u-lsn-%X-%X.spill",
+ snprintf(path, MAXPGPATH, "%s/%s/xid-%u-lsn-%X-%X.spill",
+ PG_REPLSLOT_DIR,
NameStr(MyReplicationSlot->data.name),
xid, LSN_FORMAT_ARGS(recptr));
}
@@ -4627,8 +4628,8 @@ StartupReorderBuffer(void)
DIR *logical_dir;
struct dirent *logical_de;
- logical_dir = AllocateDir("pg_replslot");
- while ((logical_de = ReadDir(logical_dir, "pg_replslot")) != NULL)
+ logical_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((logical_de = ReadDir(logical_dir, PG_REPLSLOT_DIR)) != NULL)
{
if (strcmp(logical_de->d_name, ".") == 0 ||
strcmp(logical_de->d_name, "..") == 0)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index c290339af5..2e7366ef5f 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -20,7 +20,7 @@
* on standbys (to support cascading setups). The requirement that slots be
* usable on standbys precludes storing them in the system catalogs.
*
- * Each replication slot gets its own directory inside the $PGDATA/pg_replslot
+ * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
* directory. Inside that directory the state file will contain the slot's
* own data. Additional data can be stored alongside that file if required.
* While the server is running, the state data is also cached in memory for
@@ -916,8 +916,8 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
LWLockAcquire(ReplicationSlotAllocationLock, LW_EXCLUSIVE);
/* Generate pathnames. */
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* Rename the slot directory on disk, so that we'll no longer recognize
@@ -938,7 +938,7 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
*/
START_CRIT_SECTION();
fsync_fname(tmppath, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
else
@@ -1016,7 +1016,7 @@ ReplicationSlotSave(void)
Assert(MyReplicationSlot != NULL);
- sprintf(path, "pg_replslot/%s", NameStr(MyReplicationSlot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(MyReplicationSlot->data.name));
SaveSlotToPath(MyReplicationSlot, path, ERROR);
}
@@ -1881,7 +1881,7 @@ CheckPointReplicationSlots(bool is_shutdown)
continue;
/* save the slot to disk, locking is handled in SaveSlotToPath() */
- sprintf(path, "pg_replslot/%s", NameStr(s->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(s->data.name));
/*
* Slot's data is not flushed each time the confirmed_flush LSN is
@@ -1922,8 +1922,8 @@ StartupReplicationSlots(void)
elog(DEBUG1, "starting up replication slots");
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
char path[MAXPGPATH + 12];
PGFileType de_type;
@@ -1932,7 +1932,7 @@ StartupReplicationSlots(void)
strcmp(replication_de->d_name, "..") == 0)
continue;
- snprintf(path, sizeof(path), "pg_replslot/%s", replication_de->d_name);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, replication_de->d_name);
de_type = get_dirent_type(path, replication_de, false, DEBUG1);
/* we're only creating directories here, skip if it's not our's */
@@ -1949,7 +1949,7 @@ StartupReplicationSlots(void)
path)));
continue;
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
continue;
}
@@ -1987,8 +1987,8 @@ CreateSlotOnDisk(ReplicationSlot *slot)
* takes out the lock, if we'd take the lock here, we'd deadlock.
*/
- sprintf(path, "pg_replslot/%s", NameStr(slot->data.name));
- sprintf(tmppath, "pg_replslot/%s.tmp", NameStr(slot->data.name));
+ sprintf(path, "%s/%s", PG_REPLSLOT_DIR, NameStr(slot->data.name));
+ sprintf(tmppath, "%s/%s.tmp", PG_REPLSLOT_DIR, NameStr(slot->data.name));
/*
* It's just barely possible that some previous effort to create or drop a
@@ -2027,7 +2027,7 @@ CreateSlotOnDisk(ReplicationSlot *slot)
START_CRIT_SECTION();
fsync_fname(path, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
}
@@ -2170,7 +2170,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
fsync_fname(path, false);
fsync_fname(dir, true);
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
END_CRIT_SECTION();
@@ -2205,7 +2205,7 @@ RestoreSlotFromDisk(const char *name)
/* no need to lock here, no concurrent access allowed yet */
/* delete temp file if it exists */
- sprintf(slotdir, "pg_replslot/%s", name);
+ sprintf(slotdir, "%s/%s", PG_REPLSLOT_DIR, name);
sprintf(path, "%s/state.tmp", slotdir);
if (unlink(path) < 0 && errno != ENOENT)
ereport(PANIC,
@@ -2332,7 +2332,7 @@ RestoreSlotFromDisk(const char *name)
(errmsg("could not remove directory \"%s\"",
slotdir)));
}
- fsync_fname("pg_replslot", true);
+ fsync_fname(PG_REPLSLOT_DIR, true);
return;
}
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index 0d82557417..e645e6b85a 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -708,7 +708,7 @@ pg_ls_logicalmapdir(PG_FUNCTION_ARGS)
}
/*
- * Function to return the list of files in the pg_replslot/<replication_slot>
+ * Function to return the list of files in the PG_REPLSLOT_DIR/<replication_slot>
* directory.
*/
Datum
@@ -728,6 +728,7 @@ pg_ls_replslotdir(PG_FUNCTION_ARGS)
errmsg("replication slot \"%s\" does not exist",
slotname)));
- snprintf(path, sizeof(path), "pg_replslot/%s", slotname);
+ snprintf(path, sizeof(path), "%s/%s", PG_REPLSLOT_DIR, slotname);
+
return pg_ls_dir_files(fcinfo, path, false);
}
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f00718a015..2ee55eced0 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -242,7 +242,7 @@ static const char *const subdirs[] = {
"pg_multixact/offsets",
"base",
"base/1",
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
"pg_tblspc",
"pg_stat",
"pg_stat_tmp",
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 4458324c9d..00e644d988 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -97,7 +97,7 @@ static const char *const excludeDirContents[] =
* even if the intention is to restore to another primary. See backup.sgml
* for a more detailed description.
*/
- "pg_replslot",
+ "pg_replslot", /* defined as PG_REPLSLOT_DIR */
/* Contents removed on startup, see dsm_cleanup_for_mmap(). */
"pg_dynshmem", /* defined as PG_DYNSHMEM_DIR */
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index c2ee149fd6..9665faac1c 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -17,6 +17,8 @@
#include "storage/spin.h"
#include "replication/walreceiver.h"
+#define PG_REPLSLOT_DIR "pg_replslot"
+
/*
* Behaviour of replication slots, upon release or crash.
*
--
2.34.1
--5CsiXv77sjBcVIcJ--
^ permalink raw reply [nested|flat] 30+ messages in thread
* define PG_REPLSLOT_DIR
@ 2024-08-14 11:32 Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-16 04:31 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
0 siblings, 3 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-14 11:32 UTC (permalink / raw)
To: [email protected]
Hi hackers,
while working on a replication slot tool (idea is to put it in contrib, not
shared yet), I realized that "pg_replslot" is being used > 25 times in
.c files.
I think it would make sense to replace those occurrences with a $SUBJECT, attached
a patch doing so.
There is 2 places where it is not done:
src/bin/initdb/initdb.c
src/bin/pg_rewind/filemap.c
for consistency with the existing PG_STAT_TMP_DIR define.
Out of curiosity, checking the sizes of affected files (O2, no debug):
with patch:
text data bss dec hex filename
20315 224 17 20556 504c src/backend/backup/basebackup.o
30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
23812 36 40 23888 5d50 src/backend/replication/slot.o
6367 0 0 6367 18df src/backend/utils/adt/genfile.o
40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
without patch:
text data bss dec hex filename
20315 224 17 20556 504c src/backend/backup/basebackup.o
30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
23766 36 40 23842 5d22 src/backend/replication/slot.o
6363 0 0 6363 18db src/backend/utils/adt/genfile.o
40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
Also, I think we could do the same for:
pg_notify
pg_serial
pg_subtrans
pg_wal
pg_multixact
pg_tblspc
pg_logical
And I volunteer to do so if you think that makes sense.
Looking forward to your feedback,
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-16 00:40 ` Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2 siblings, 1 reply; 30+ messages in thread
From: Alvaro Herrera @ 2024-08-16 00:40 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On 2024-Aug-14, Bertrand Drouvot wrote:
> Out of curiosity, checking the sizes of affected files (O2, no debug):
>
> with patch:
>
> text data bss dec hex filename
> 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
> without patch:
> 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
Hmm, I suppose this delta can be explained because because the literal
string is used inside larger snprintf() format strings or similar, so
things like
snprintf(path, sizeof(path),
- "pg_replslot/%s/%s", slotname,
+ "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
spill_de->d_name);
and
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
- path, slotname)));
+ errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
+ PG_REPLSLOT_DIR, path, slotname)));
I don't disagree with making this change, but changing some of the other
directory names you suggest, such as
> pg_notify
> pg_serial
> pg_subtrans
> pg_multixact
> pg_wal
would probably make no difference, since there are no literal strings
that contain that as a substring(*). It might some sense to handle
pg_tblspc similarly. As for pg_logical, I think you'll want separate
defines for pg_logical/mappings, pg_logical/snapshots and so on.
(*) I hope you're not going to suggest this kind of change (word-diff):
if (GetOldestSnapshot())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("[-pg_wal-]{+%s+}_replay_wait() must be only called without an active or registered snapshot"{+, PG_WAL_DIR+}),
errdetail("Make sure [-pg_wal-]{+%s+}_replay_wait() isn't called within a transaction with an isolation level higher than READ COMMITTED, another procedure, or a function."{+, PG_WAL_DIR+})));
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia." (El principio Dilbert)
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
@ 2024-08-19 14:11 ` Bertrand Drouvot <[email protected]>
2024-08-20 08:47 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
0 siblings, 2 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-19 14:11 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: [email protected]
Hi,
On Thu, Aug 15, 2024 at 08:40:42PM -0400, Alvaro Herrera wrote:
> On 2024-Aug-14, Bertrand Drouvot wrote:
>
> > Out of curiosity, checking the sizes of affected files (O2, no debug):
> >
> > with patch:
> >
> > text data bss dec hex filename
> > 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
>
> > without patch:
> > 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
>
> Hmm, I suppose this delta can be explained because because the literal
> string is used inside larger snprintf() format strings or similar, so
> things like
>
> snprintf(path, sizeof(path),
> - "pg_replslot/%s/%s", slotname,
> + "%s/%s/%s", PG_REPLSLOT_DIR, slotname,
> spill_de->d_name);
>
> and
>
> ereport(ERROR,
> (errcode_for_file_access(),
> - errmsg("could not remove file \"%s\" during removal of pg_replslot/%s/xid*: %m",
> - path, slotname)));
> + errmsg("could not remove file \"%s\" during removal of %s/%s/xid*: %m",
> + PG_REPLSLOT_DIR, path, slotname)));
>
I did not look in details, but yeah that could probably be explained that way.
> I don't disagree with making this change, but changing some of the other
> directory names you suggest, such as
>
> > pg_notify
> > pg_serial
> > pg_subtrans
> > pg_multixact
> > pg_wal
>
> would probably make no difference, since there are no literal strings
> that contain that as a substring(*). It might some sense to handle
> pg_tblspc similarly. As for pg_logical, I think you'll want separate
> defines for pg_logical/mappings, pg_logical/snapshots and so on.
>
>
> (*) I hope you're not going to suggest this kind of change (word-diff):
>
> if (GetOldestSnapshot())
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("[-pg_wal-]{+%s+}_replay_wait() must be only called without an active or registered snapshot"{+, PG_WAL_DIR+}),
> errdetail("Make sure [-pg_wal-]{+%s+}_replay_wait() isn't called within a transaction with an isolation level higher than READ COMMITTED, another procedure, or a function."{+, PG_WAL_DIR+})));
>
Yeah, would not cross my mind ;-). I might have been tempted to do the change
in pg_combinebackup.c though.
Having said that, I agree to focus only on:
pg_replslot
pg_tblspc
pg_logical/*
I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences
are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any
case.
Please find attached the related patches.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-20 08:47 ` Michael Paquier <[email protected]>
2024-08-20 12:16 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 12:30 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
1 sibling, 2 replies; 30+ messages in thread
From: Michael Paquier @ 2024-08-20 08:47 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
On Mon, Aug 19, 2024 at 02:11:55PM +0000, Bertrand Drouvot wrote:
> I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences
> are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any
> case.
>
> Please find attached the related patches.
No real objection about the replslot and pg_logical bits.
- * $PGDATA/pg_tblspc/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
+ * $PGDATA/PG_TBLSPC_DIR/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
For the tablespace parts, I am not sure that I would update the
comments to reflect the variables, TBH. Somebody reading the comments
would need to refer back to pg_tblspc/ in the header.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 08:47 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
@ 2024-08-20 12:16 ` Bertrand Drouvot <[email protected]>
1 sibling, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 12:16 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
Hi,
On Tue, Aug 20, 2024 at 05:47:57PM +0900, Michael Paquier wrote:
> On Mon, Aug 19, 2024 at 02:11:55PM +0000, Bertrand Drouvot wrote:
> > I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences
> > are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any
> > case.
> >
> > Please find attached the related patches.
>
> No real objection about the replslot and pg_logical bits.
Thanks for looking at it!
>
> - * $PGDATA/pg_tblspc/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
> + * $PGDATA/PG_TBLSPC_DIR/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
>
> For the tablespace parts, I am not sure that I would update the
> comments to reflect the variables, TBH. Somebody reading the comments
> would need to refer back to pg_tblspc/ in the header.
I'm not sure as, for example, for PG_STAT_TMP_DIR we have those ones:
src/backend/backup/basebackup.c: * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped
src/bin/pg_rewind/filemap.c: * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped
so I thought it would be better to be consistent.
That said, I don't have a strong opinion about it, but then I guess you'd want to
do the same for the ones related to replslot:
src/backend/replication/slot.c: * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_REPLSLOT_DIR/<replication_slot>
and pg_logical:
src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_LOGICAL_SNAPSHOTS_DIR directory.
src/backend/utils/adt/genfile.c: * Function to return the list of files in the PG_LOGICAL_MAPPINGS_DIR directory.
, right?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 08:47 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
@ 2024-08-20 12:30 ` Yugo Nagata <[email protected]>
2024-08-20 16:26 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Yugo Nagata @ 2024-08-20 12:30 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
On Tue, 20 Aug 2024 17:47:57 +0900
Michael Paquier <[email protected]> wrote:
> On Mon, Aug 19, 2024 at 02:11:55PM +0000, Bertrand Drouvot wrote:
> > I made the changes for pg_tblspc in pg_combinebackup.c as the number of occurences
> > are greater that the "pg_wal" ones and we were to define PG_TBLSPC_DIR in any
> > case.
> >
> > Please find attached the related patches.
>
> No real objection about the replslot and pg_logical bits.
>
> - * $PGDATA/pg_tblspc/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
> + * $PGDATA/PG_TBLSPC_DIR/spcoid/PG_MAJORVER_CATVER/dboid/relfilenumber
>
> For the tablespace parts, I am not sure that I would update the
> comments to reflect the variables, TBH. Somebody reading the comments
> would need to refer back to pg_tblspc/ in the header.
I also think that it is not necessary to change the comments even for pg_replslot.
- * Each replication slot gets its own directory inside the $PGDATA/pg_replslot
+ * Each replication slot gets its own directory inside the $PGDATA/PG_REPLSLOT_DIR
For example, I found that comments in xlog.c use "pg_wal" even though XLOGDIR is used
in the codes as below, and I don't feel any problem for this.
> static void
> ValidateXLOGDirectoryStructure(void)
> {
> char path[MAXPGPATH];
> struct stat stat_buf;
>
> /* Check for pg_wal; if it doesn't exist, error out */
> if (stat(XLOGDIR, &stat_buf) != 0 ||
> !S_ISDIR(stat_buf.st_mode))
Should be the follwing also rewritten using sizeof(PG_REPLSLOT_DIR)?
struct stat statbuf;
char path[MAXPGPATH * 2 + 12];
Regards,
Yugo Nagata
--
Yugo Nagata <[email protected]>
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 08:47 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:30 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
@ 2024-08-20 16:26 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 16:26 UTC (permalink / raw)
To: Yugo Nagata <[email protected]>; +Cc: Michael Paquier <[email protected]>; Alvaro Herrera <[email protected]>; [email protected]
Hi,
On Tue, Aug 20, 2024 at 09:30:48PM +0900, Yugo Nagata wrote:
> Should be the follwing also rewritten using sizeof(PG_REPLSLOT_DIR)?
>
> struct stat statbuf;
> char path[MAXPGPATH * 2 + 12];
>
>
Yeah, done in v3 that I just shared up-thread (also removed the macros from
the comments).
Thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-20 14:15 ` Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 30+ messages in thread
From: Alvaro Herrera @ 2024-08-20 14:15 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On 2024-Aug-19, Bertrand Drouvot wrote:
> diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
> index 6f006d5a93..a6cb091635 100644
> --- a/src/include/common/relpath.h
> +++ b/src/include/common/relpath.h
> @@ -33,6 +33,10 @@ typedef Oid RelFileNumber;
> #define TABLESPACE_VERSION_DIRECTORY "PG_" PG_MAJORVERSION "_" \
> CppAsString2(CATALOG_VERSION_NO)
>
> +#define PG_TBLSPC_DIR "pg_tblspc"
This one is missing some commentary along the lines of "This must not be
changed, unless you want to break every tool in the universe". As is,
it's quite tempting.
> +#define PG_TBLSPC_DIR_SLASH PG_TBLSPC_DIR "/"
I would make this simply "pg_tblspc/", since it's not really possible to
change pg_tblspc anyway. Also, have a comment explaining why we have
it.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
@ 2024-08-20 16:23 ` Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 16:23 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: [email protected]
Hi,
On Tue, Aug 20, 2024 at 10:15:44AM -0400, Alvaro Herrera wrote:
> On 2024-Aug-19, Bertrand Drouvot wrote:
>
> > diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
> > index 6f006d5a93..a6cb091635 100644
> > --- a/src/include/common/relpath.h
> > +++ b/src/include/common/relpath.h
> > @@ -33,6 +33,10 @@ typedef Oid RelFileNumber;
> > #define TABLESPACE_VERSION_DIRECTORY "PG_" PG_MAJORVERSION "_" \
> > CppAsString2(CATALOG_VERSION_NO)
> >
> > +#define PG_TBLSPC_DIR "pg_tblspc"
>
> This one is missing some commentary along the lines of "This must not be
> changed, unless you want to break every tool in the universe". As is,
> it's quite tempting.
Yeah, makes sense, thanks.
> > +#define PG_TBLSPC_DIR_SLASH PG_TBLSPC_DIR "/"
>
> I would make this simply "pg_tblspc/", since it's not really possible to
> change pg_tblspc anyway. Also, have a comment explaining why we have
> it.
Please find attached v3 that:
- takes care of your comments (and also removed the use of PG_TBLSPC_DIR in
RELATIVE_PG_TBLSPC_DIR).
- removes the new macros from the comments (see Michael's and Yugo-San's
comments in [0] resp. [1]).
- adds a missing sizeof() (see [1]).
- implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). It's
done in 0002 (I used REPLSLOT_DIR_ARGS though).
- fixed a macro usage in ReorderBufferCleanupSerializedTXNs() (was not at the
right location, discovered while implementing 0002).
[0]: https://www.postgresql.org/message-id/ZsRYPcOtoqbWzjGG%40paquier.xyz
[1]: https://www.postgresql.org/message-id/20240820213048.207aade6a75e0dc1fe4d1067%40sraoss.co.jp
[2]: https://www.postgresql.org/message-id/CAExHW5vkjxuvyQ1fPPnuDW4nAT5jqox09ie36kciOV2%2BrhjbHA%40mail.g...
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-30 06:34 ` Michael Paquier <[email protected]>
2024-08-30 12:21 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Michael Paquier @ 2024-08-30 06:34 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
On Tue, Aug 20, 2024 at 04:23:06PM +0000, Bertrand Drouvot wrote:
> Please find attached v3 that:
>
> - takes care of your comments (and also removed the use of PG_TBLSPC_DIR in
> RELATIVE_PG_TBLSPC_DIR).
> - removes the new macros from the comments (see Michael's and Yugo-San's
> comments in [0] resp. [1]).
> - adds a missing sizeof() (see [1]).
> - implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). It's
> done in 0002 (I used REPLSLOT_DIR_ARGS though).
> - fixed a macro usage in ReorderBufferCleanupSerializedTXNs() (was not at the
> right location, discovered while implementing 0002).
I was looking at that, and applied 0001 for pg_replslot and merged
0003~0005 together for pg_logical. For the first one, at the end I
have updated the comments in genfile.c. For the second one, it looked
a bit strange to ignore "pg_logical/", which is the base for all the
others, so I have added a variable for it. Locating them in
reorderbuffer.h with the GUCs was OK, but perhaps there was an
argument for logical.h. The paths in origin.c refer to files, not
directories.
Not sure that 0002 is an improvement overall, so I have left this part
out.
In 0006, I am not sure that RELATIVE_PG_TBLSPC_DIR is really something
we should have. Sure, that's a special case for base backups when
sending a directory, but we have also pg_wal/ and its XLOGDIR so
that's inconsistent. Perhaps this part should be left as-is.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
@ 2024-08-30 12:21 ` Bertrand Drouvot <[email protected]>
2024-09-03 00:15 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-30 12:21 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
Hi,
On Fri, Aug 30, 2024 at 03:34:56PM +0900, Michael Paquier wrote:
> On Tue, Aug 20, 2024 at 04:23:06PM +0000, Bertrand Drouvot wrote:
> > Please find attached v3 that:
> >
> > - takes care of your comments (and also removed the use of PG_TBLSPC_DIR in
> > RELATIVE_PG_TBLSPC_DIR).
> > - removes the new macros from the comments (see Michael's and Yugo-San's
> > comments in [0] resp. [1]).
> > - adds a missing sizeof() (see [1]).
> > - implements Ashutosh's idea of adding a new SLOT_DIRNAME_ARGS (see [2]). It's
> > done in 0002 (I used REPLSLOT_DIR_ARGS though).
> > - fixed a macro usage in ReorderBufferCleanupSerializedTXNs() (was not at the
> > right location, discovered while implementing 0002).
>
> I was looking at that, and applied 0001 for pg_replslot and merged
> 0003~0005 together for pg_logical.
Thanks!
> In 0006, I am not sure that RELATIVE_PG_TBLSPC_DIR is really something
> we should have. Sure, that's a special case for base backups when
> sending a directory, but we have also pg_wal/ and its XLOGDIR so
> that's inconsistent.
That's right but OTOH there is no (for good reason) PG_WAL_DIR or such defined.
That said, I don't have a strong opinion on this one, I think that also makes
sense to leave it as it is. Please find attached v4 doing so.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-30 12:21 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-09-03 00:15 ` Michael Paquier <[email protected]>
2024-09-03 04:35 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Michael Paquier @ 2024-09-03 00:15 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
On Fri, Aug 30, 2024 at 12:21:29PM +0000, Bertrand Drouvot wrote:
> That said, I don't have a strong opinion on this one, I think that also makes
> sense to leave it as it is. Please find attached v4 doing so.
The changes in astreamer_file.c are actually wrong regarding the fact
that should_allow_existing_directory() needs to be able to work with
the branch where this code is located as well as back-branches,
because pg_basebackup from version N supports ~(N-1) versions down to
a certain version, so changing it is not right. This is why pg_xlog
and pg_wal are both listed there.
Perhaps we should to more for the two entries in basebackup.c with the
relative paths, but I'm not sure that's worth bothering, either. At
the end, I got no objections about the remaining pieces, so applied.
How do people feel about the suggestions to update the comments at the
end? With the comment in relpath.h suggesting to not change that, the
current state of HEAD is fine by me.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-30 12:21 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-09-03 00:15 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
@ 2024-09-03 04:35 ` Bertrand Drouvot <[email protected]>
2024-09-03 06:27 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-09-03 04:35 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
Hi,
On Tue, Sep 03, 2024 at 09:15:50AM +0900, Michael Paquier wrote:
> On Fri, Aug 30, 2024 at 12:21:29PM +0000, Bertrand Drouvot wrote:
> > That said, I don't have a strong opinion on this one, I think that also makes
> > sense to leave it as it is. Please find attached v4 doing so.
>
> The changes in astreamer_file.c are actually wrong regarding the fact
> that should_allow_existing_directory() needs to be able to work with
> the branch where this code is located as well as back-branches,
> because pg_basebackup from version N supports ~(N-1) versions down to
> a certain version, so changing it is not right. This is why pg_xlog
> and pg_wal are both listed there.
I understand why pg_xlog and pg_wal both need to be listed here, but I don't
get why the proposed changes were "wrong". Or, are you saying that if for any
reason PG_TBLSPC_DIR needs to be changed that would not work anymore? If
that's the case, then I guess we'd have to add a new define and test like:
strcmp(filename, PG_TBLSPC_DIR) == 0) || strcmp(filename, NEW_PG_TBLSPC_DIR) == 0)
, no?
The question is more out of curiosity, not saying the changes should be applied
in astreamer_file.c though.
> Perhaps we should to more for the two entries in basebackup.c with the
> relative paths, but I'm not sure that's worth bothering, either.
I don't have a strong opinon on those ones.
> At
> the end, I got no objections about the remaining pieces, so applied.
Thanks!
> How do people feel about the suggestions to update the comments at the
> end? With the comment in relpath.h suggesting to not change that, the
> current state of HEAD is fine by me.
Yeah, I think that's fine and specially because there is still some places (
outside of the comments) that don't rely on the define(s).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-30 12:21 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-09-03 00:15 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-09-03 04:35 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-09-03 06:27 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Michael Paquier @ 2024-09-03 06:27 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; [email protected]
On Tue, Sep 03, 2024 at 04:35:28AM +0000, Bertrand Drouvot wrote:
> I understand why pg_xlog and pg_wal both need to be listed here, but I don't
> get why the proposed changes were "wrong". Or, are you saying that if for any
> reason PG_TBLSPC_DIR needs to be changed that would not work
> anymore?
Yes. Folks are not going to do that, but changing PG_TBLSPC_DIR would
change the decision-making when streaming from versions older than
v17 with clients tools from v18~.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-16 04:31 ` Yugo Nagata <[email protected]>
2024-08-19 14:13 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2 siblings, 1 reply; 30+ messages in thread
From: Yugo Nagata @ 2024-08-16 04:31 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Wed, 14 Aug 2024 11:32:14 +0000
Bertrand Drouvot <[email protected]> wrote:
> Hi hackers,
>
> while working on a replication slot tool (idea is to put it in contrib, not
> shared yet), I realized that "pg_replslot" is being used > 25 times in
> .c files.
>
> I think it would make sense to replace those occurrences with a $SUBJECT, attached
> a patch doing so.
>
> There is 2 places where it is not done:
>
> src/bin/initdb/initdb.c
> src/bin/pg_rewind/filemap.c
>
> for consistency with the existing PG_STAT_TMP_DIR define.
>
> Out of curiosity, checking the sizes of affected files (O2, no debug):
>
> with patch:
>
> text data bss dec hex filename
> 20315 224 17 20556 504c src/backend/backup/basebackup.o
> 30304 0 8 30312 7668 src/backend/replication/logical/reorderbuffer.o
> 23812 36 40 23888 5d50 src/backend/replication/slot.o
> 6367 0 0 6367 18df src/backend/utils/adt/genfile.o
> 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
> 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
>
> without patch:
>
> text data bss dec hex filename
> 20315 224 17 20556 504c src/backend/backup/basebackup.o
> 30286 0 8 30294 7656 src/backend/replication/logical/reorderbuffer.o
> 23766 36 40 23842 5d22 src/backend/replication/slot.o
> 6363 0 0 6363 18db src/backend/utils/adt/genfile.o
> 40997 2574 2528 46099 b413 src/bin/initdb/initdb.o
> 6963 224 8 7195 1c1b src/bin/pg_rewind/filemap.o
>
> Also, I think we could do the same for:
>
> pg_notify
> pg_serial
> pg_subtrans
> pg_wal
> pg_multixact
> pg_tblspc
> pg_logical
>
> And I volunteer to do so if you think that makes sense.
>
> Looking forward to your feedback,
/* restore all slots by iterating over all on-disk entries */
- replication_dir = AllocateDir("pg_replslot");
- while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
+ replication_dir = AllocateDir(PG_REPLSLOT_DIR);
+ while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
{
char path[MAXPGPATH + 12];
PGFileType de_type;
I think the size of path can be rewritten to "MAXPGPATH + sizeof(PG_REPLSLOT_DIR)"
and it seems easier to understand the reason why this size is used.
(That said, I wonder the path would never longer than MAXPGPATH...)
Regards,
Yugo Nagata
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
--
Yugo Nagata <[email protected]>
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 04:31 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
@ 2024-08-19 14:13 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-19 14:13 UTC (permalink / raw)
To: Yugo Nagata <[email protected]>; +Cc: [email protected]
Hi,
On Fri, Aug 16, 2024 at 01:31:11PM +0900, Yugo Nagata wrote:
> On Wed, 14 Aug 2024 11:32:14 +0000
> Bertrand Drouvot <[email protected]> wrote:
> > Looking forward to your feedback,
>
> /* restore all slots by iterating over all on-disk entries */
> - replication_dir = AllocateDir("pg_replslot");
> - while ((replication_de = ReadDir(replication_dir, "pg_replslot")) != NULL)
> + replication_dir = AllocateDir(PG_REPLSLOT_DIR);
> + while ((replication_de = ReadDir(replication_dir, PG_REPLSLOT_DIR)) != NULL)
> {
> char path[MAXPGPATH + 12];
> PGFileType de_type;
>
> I think the size of path can be rewritten to "MAXPGPATH + sizeof(PG_REPLSLOT_DIR)"
> and it seems easier to understand the reason why this size is used.
Thanks for the feedback!
Yeah, fully agree, it has been done that way in v2 that I just shared up-thread.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-19 10:41 ` Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2 siblings, 1 reply; 30+ messages in thread
From: Ashutosh Bapat @ 2024-08-19 10:41 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi hackers,
>
> while working on a replication slot tool (idea is to put it in contrib, not
> shared yet), I realized that "pg_replslot" is being used > 25 times in
> .c files.
>
> I think it would make sense to replace those occurrences with a $SUBJECT, attached
> a patch doing so.
Many of these places are slot specific directory/file names within
pg_replslot. I think we can further improve the code by creating macro
on the lines of LSN_FORMAT_ARGS
#define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname)
this way we "codify" method to construct the slot directory name
everywhere. We may add another macro
#define SLOT_DIRNAME_FORMAT "%s/%s" to further enforce the same. But
I didn't find similar LSN_FORMAT macro defined as "%X/%X". I don't
remember exactly why we didn't add it. May be because of trouble with
translations.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
@ 2024-08-19 14:17 ` Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-19 14:17 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: [email protected]
Hi,
On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote:
> On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot
> <[email protected]> wrote:
> >
> > Hi hackers,
> >
> > while working on a replication slot tool (idea is to put it in contrib, not
> > shared yet), I realized that "pg_replslot" is being used > 25 times in
> > .c files.
> >
> > I think it would make sense to replace those occurrences with a $SUBJECT, attached
> > a patch doing so.
>
> Many of these places are slot specific directory/file names within
> pg_replslot. I think we can further improve the code by creating macro
> on the lines of LSN_FORMAT_ARGS
> #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname)
> this way we "codify" method to construct the slot directory name
> everywhere.
Thanks for the feedback!
I think that could make sense. As the already proposed mechanical changes are
error prone (from my point of view), I would suggest to have a look at your
proposal once the proposed changes go in.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-20 03:56 ` Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Ashutosh Bapat @ 2024-08-20 03:56 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote:
> > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot
> > <[email protected]> wrote:
> > >
> > > Hi hackers,
> > >
> > > while working on a replication slot tool (idea is to put it in contrib, not
> > > shared yet), I realized that "pg_replslot" is being used > 25 times in
> > > .c files.
> > >
> > > I think it would make sense to replace those occurrences with a $SUBJECT, attached
> > > a patch doing so.
> >
> > Many of these places are slot specific directory/file names within
> > pg_replslot. I think we can further improve the code by creating macro
> > on the lines of LSN_FORMAT_ARGS
> > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname)
> > this way we "codify" method to construct the slot directory name
> > everywhere.
>
> Thanks for the feedback!
>
> I think that could make sense. As the already proposed mechanical changes are
> error prone (from my point of view), I would suggest to have a look at your
> proposal once the proposed changes go in.
What do you mean by error prone?
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
@ 2024-08-20 05:19 ` Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 05:19 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: [email protected]
Hi,
On Tue, Aug 20, 2024 at 09:26:59AM +0530, Ashutosh Bapat wrote:
> On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote:
> > > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot
> > > <[email protected]> wrote:
> > > >
> > > > Hi hackers,
> > > >
> > > > while working on a replication slot tool (idea is to put it in contrib, not
> > > > shared yet), I realized that "pg_replslot" is being used > 25 times in
> > > > .c files.
> > > >
> > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached
> > > > a patch doing so.
> > >
> > > Many of these places are slot specific directory/file names within
> > > pg_replslot. I think we can further improve the code by creating macro
> > > on the lines of LSN_FORMAT_ARGS
> > > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname)
> > > this way we "codify" method to construct the slot directory name
> > > everywhere.
> >
> > Thanks for the feedback!
> >
> > I think that could make sense. As the already proposed mechanical changes are
> > error prone (from my point of view), I would suggest to have a look at your
> > proposal once the proposed changes go in.
>
> What do you mean by error prone?
I meant to say that it is "easy" to make mistakes when doing those manual
mechanical changes. Also I think it's not that fun/easy to review, that's why
I think it's better to do one change at a time. Does that make sense to you?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-20 05:40 ` Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Ashutosh Bapat @ 2024-08-20 05:40 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
On Tue, Aug 20, 2024 at 10:49 AM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Tue, Aug 20, 2024 at 09:26:59AM +0530, Ashutosh Bapat wrote:
> > On Mon, Aug 19, 2024 at 7:47 PM Bertrand Drouvot
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Aug 19, 2024 at 04:11:31PM +0530, Ashutosh Bapat wrote:
> > > > On Wed, Aug 14, 2024 at 5:02 PM Bertrand Drouvot
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi hackers,
> > > > >
> > > > > while working on a replication slot tool (idea is to put it in contrib, not
> > > > > shared yet), I realized that "pg_replslot" is being used > 25 times in
> > > > > .c files.
> > > > >
> > > > > I think it would make sense to replace those occurrences with a $SUBJECT, attached
> > > > > a patch doing so.
> > > >
> > > > Many of these places are slot specific directory/file names within
> > > > pg_replslot. I think we can further improve the code by creating macro
> > > > on the lines of LSN_FORMAT_ARGS
> > > > #define SLOT_DIRNAME_ARGS(slotname) (PG_REPLSLOT_DIR, slotname)
> > > > this way we "codify" method to construct the slot directory name
> > > > everywhere.
> > >
> > > Thanks for the feedback!
> > >
> > > I think that could make sense. As the already proposed mechanical changes are
> > > error prone (from my point of view), I would suggest to have a look at your
> > > proposal once the proposed changes go in.
> >
> > What do you mean by error prone?
>
> I meant to say that it is "easy" to make mistakes when doing those manual
> mechanical changes. Also I think it's not that fun/easy to review, that's why
> I think it's better to do one change at a time. Does that make sense to you?
Since these are all related changes, doing them at once might make it
faster. You may use multiple commits (one for each change) to
"contain" errors.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
@ 2024-08-20 08:41 ` Michael Paquier <[email protected]>
2024-08-20 12:06 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Michael Paquier @ 2024-08-20 08:41 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; [email protected]
On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote:
> Since these are all related changes, doing them at once might make it
> faster. You may use multiple commits (one for each change)
Doing multiple commits with individual definitions for each path would
be the way to go for me. All that is mechanical, still that feels
slightly cleaner.
> to "contain" errors.
I am not sure what you mean by that.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
@ 2024-08-20 12:06 ` Bertrand Drouvot <[email protected]>
2024-08-20 16:28 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 12:06 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected]
Hi,
On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote:
> On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote:
> > Since these are all related changes, doing them at once might make it
> > faster. You may use multiple commits (one for each change)
>
> Doing multiple commits with individual definitions for each path would
> be the way to go for me. All that is mechanical, still that feels
> slightly cleaner.
Right, that's what v2 has done. If there is a need for v3 then I'll add one
dedicated patch for Ashutosh's proposal in the v3 series.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:06 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-20 16:28 ` Bertrand Drouvot <[email protected]>
2024-08-21 04:44 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-20 16:28 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected]
Hi,
On Tue, Aug 20, 2024 at 12:06:52PM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote:
> > On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote:
> > > Since these are all related changes, doing them at once might make it
> > > faster. You may use multiple commits (one for each change)
> >
> > Doing multiple commits with individual definitions for each path would
> > be the way to go for me. All that is mechanical, still that feels
> > slightly cleaner.
>
> Right, that's what v2 has done. If there is a need for v3 then I'll add one
> dedicated patch for Ashutosh's proposal in the v3 series.
Ashutosh's idea is implemented in v3 that I just shared up-thread (I used
REPLSLOT_DIR_ARGS though).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:06 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 16:28 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
@ 2024-08-21 04:44 ` Ashutosh Bapat <[email protected]>
2024-08-21 05:03 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
0 siblings, 1 reply; 30+ messages in thread
From: Ashutosh Bapat @ 2024-08-21 04:44 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]
If this isn't in commitfest already, please add it there.
On Tue, Aug 20, 2024 at 9:58 PM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Tue, Aug 20, 2024 at 12:06:52PM +0000, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Tue, Aug 20, 2024 at 05:41:48PM +0900, Michael Paquier wrote:
> > > On Tue, Aug 20, 2024 at 11:10:46AM +0530, Ashutosh Bapat wrote:
> > > > Since these are all related changes, doing them at once might make it
> > > > faster. You may use multiple commits (one for each change)
> > >
> > > Doing multiple commits with individual definitions for each path would
> > > be the way to go for me. All that is mechanical, still that feels
> > > slightly cleaner.
> >
> > Right, that's what v2 has done. If there is a need for v3 then I'll add one
> > dedicated patch for Ashutosh's proposal in the v3 series.
>
> Ashutosh's idea is implemented in v3 that I just shared up-thread (I used
> REPLSLOT_DIR_ARGS though).
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 30+ messages in thread
* Re: define PG_REPLSLOT_DIR
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:06 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 16:28 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-21 04:44 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
@ 2024-08-21 05:03 ` Bertrand Drouvot <[email protected]>
0 siblings, 0 replies; 30+ messages in thread
From: Bertrand Drouvot @ 2024-08-21 05:03 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; +Cc: Michael Paquier <[email protected]>; [email protected]
Hi,
On Wed, Aug 21, 2024 at 10:14:20AM +0530, Ashutosh Bapat wrote:
> If this isn't in commitfest already, please add it there.
>
Done in [0].
[0]: https://commitfest.postgresql.org/49/5193/
Thanks!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 30+ messages in thread
end of thread, other threads:[~2024-09-03 06:27 UTC | newest]
Thread overview: 30+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 15:53 [PATCH v48 3/3] Replace assuming a composite object on a scalar Dmitrii Dolgov <[email protected]>
2024-08-14 09:16 [PATCH v2 1/5] Define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-14 09:16 [PATCH v3 1/6] Define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-14 09:16 [PATCH v1] Define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-14 11:32 define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-16 00:40 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-19 14:11 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 08:47 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:16 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 12:30 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
2024-08-20 16:26 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 14:15 ` Re: define PG_REPLSLOT_DIR Alvaro Herrera <[email protected]>
2024-08-20 16:23 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-30 06:34 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-30 12:21 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-09-03 00:15 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-09-03 04:35 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-09-03 06:27 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-16 04:31 ` Re: define PG_REPLSLOT_DIR Yugo Nagata <[email protected]>
2024-08-19 14:13 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-19 10:41 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-19 14:17 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 03:56 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 05:19 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 05:40 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-20 08:41 ` Re: define PG_REPLSLOT_DIR Michael Paquier <[email protected]>
2024-08-20 12:06 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-20 16:28 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
2024-08-21 04:44 ` Re: define PG_REPLSLOT_DIR Ashutosh Bapat <[email protected]>
2024-08-21 05:03 ` Re: define PG_REPLSLOT_DIR Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox