logical decoding/replication: new functions pg_ls_logicaldir and pg_ls_replslotdir

On Fri, Oct 8, 2021 at 4:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

At times, users want to know what are the files (snapshot and mapping
files) that are available under pg_logical directory and also the
spill files that are under pg_replslot directory and how much space
they occupy.

Why can't you use pg_ls_dir to see the contents of pg_replslot? To
know the space taken by spilling, you might want to check
pg_stat_replication_slots[1]https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW as that gives information about
spill_bytes.

[1]: https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW

--
With Regards,
Amit Kapila.

On Fri, Oct 22, 2021 at 3:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Oct 8, 2021 at 4:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

At times, users want to know what are the files (snapshot and mapping
files) that are available under pg_logical directory and also the
spill files that are under pg_replslot directory and how much space
they occupy.

Why can't you use pg_ls_dir to see the contents of pg_replslot? To
know the space taken by spilling, you might want to check
pg_stat_replication_slots[1] as that gives information about
spill_bytes.

[1] - https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW

Thanks Amit!

pg_ls_dir gives the list of directories and files, but not their
sizes. And it looks like the spill_bytes from
pg_stat_replication_slots is the accumulated byte count (see [1]postgres=# select pg_ls_dir('/home/bharath/postgres/inst/bin/data/pg_replslot/mysub'); pg_ls_dir ----------- state (1 row)), not
the current size of the spills files, so it's not representing the
spill files and their size at that moment.

If we have pg_ls_logicaldir and pg_ls_replslotdir returning the
files, szies, and last modified times, it will be useful in production
environments to see the disk usage of those files at the current
moment. The data from these functions can be fed to an external
analytics tool invoking the functions at regular intervals of time and
report the disk usage of these folders. This will be super useful to
analyze the questions like: Was the disk usage more at time t1? What
happened to my database system at that time? etc. And, these
functions can run independent of the stats collector process which is
currently required for the pg_stat_replication_slots view.

Thoughts?

I plan to work on a patch if okay.

[1]: postgres=# select pg_ls_dir('/home/bharath/postgres/inst/bin/data/pg_replslot/mysub'); pg_ls_dir ----------- state (1 row)
postgres=# select
pg_ls_dir('/home/bharath/postgres/inst/bin/data/pg_replslot/mysub');
pg_ls_dir
-----------
state
(1 row)

postgres=# select * from pg_stat_replication_slots;
slot_name | spill_txns | spill_count | spill_bytes | stream_txns |
stream_count | stream_bytes | total_txns | total_bytes | stats_reset
-----------+------------+-------------+-------------+-------------+--------------+--------------+------------+-------------+-------------
mysub | 3 | 6 | 396000000 | 0 |
0 | 0 | 5 | 396001128 |
(1 row)

Regards,
Bharath Rupireddy.

On Fri, Oct 22, 2021 at 04:18:04PM +0530, Bharath Rupireddy wrote:

On Fri, Oct 22, 2021 at 3:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Oct 8, 2021 at 4:39 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

At times, users want to know what are the files (snapshot and mapping
files) that are available under pg_logical directory and also the
spill files that are under pg_replslot directory and how much space
they occupy.

Why can't you use pg_ls_dir to see the contents of pg_replslot? To

Thanks Amit!

pg_ls_dir gives the list of directories and files, but not their sizes.

Returning sizes is already possible by using pg_stat_file:

ts=# SELECT dd, a, ls, stat.* FROM (SELECT current_setting('data_directory') AS dd, 'pg_logical' AS a) AS a, pg_ls_dir(a) AS ls, pg_stat_file(dd ||'/'|| a ||'/'|| ls) AS stat ;
dd | a | ls | size | access | modification | change | creation | isdir
------------------------+------------+-----------------------+------+------------------------+------------------------+------------------------+----------+-------
/var/lib/pgsql/14/data | pg_logical | replorigin_checkpoint | 8 | 2021-10-22 08:20:30-06 | 2021-10-22 08:20:30-06 | 2021-10-22 08:20:30-06 | | f
/var/lib/pgsql/14/data | pg_logical | mappings | 4096 | 2021-10-21 19:54:19-06 | 2021-10-15 19:50:35-06 | 2021-10-15 19:50:35-06 | | t
/var/lib/pgsql/14/data | pg_logical | snapshots | 4096 | 2021-10-21 19:54:19-06 | 2021-10-15 19:50:35-06 | 2021-10-15 19:50:35-06 | | t

I agree that this isn't a very friendly query, so I had created a patch adding
pg_ls_dir_metadata():
https://commitfest.postgresql.org/33/2377/

postgres=# SELECT * FROM pg_ls_dir_metadata('pg_logical');
filename | size | access | modification | change | creation | type | path
-----------------------+------+------------------------+------------------------+------------------------+----------+------+----------------------------------
mappings | 4096 | 2021-10-22 09:15:29-05 | 2021-10-22 09:15:29-05 | 2021-10-22 09:15:29-05 | | d | pg_logical/mappings
replorigin_checkpoint | 8 | 2021-10-22 09:15:47-05 | 2021-10-22 09:15:45-05 | 2021-10-22 09:15:45-05 | | - | pg_logical/replorigin_checkpoint
. | 4096 | 2021-10-22 09:16:23-05 | 2021-10-22 09:15:45-05 | 2021-10-22 09:15:45-05 | | d | pg_logical/.
.. | 4096 | 2021-10-22 09:16:01-05 | 2021-10-22 09:15:47-05 | 2021-10-22 09:15:47-05 | | d | pg_logical/..
snapshots | 4096 | 2021-10-22 09:15:29-05 | 2021-10-22 09:15:29-05 | 2021-10-22 09:15:29-05 | | d | pg_logical/snapshots
(5 rows)

I concluded that it's better to add a function to list metadata of an arbitrary
dir, rather than adding more functions to handle specific, hardcoded dirs:
/messages/by-id/20191227170220.GE12890@telsasoft.com

--
Justin

On Fri, Oct 22, 2021 at 9:26 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

I concluded that it's better to add a function to list metadata of an arbitrary
dir, rather than adding more functions to handle specific, hardcoded dirs:
/messages/by-id/20191227170220.GE12890@telsasoft.com

I just had a quick look at the pg_ls_dir_metadata() patch(I didn't
look at the other patches). While it's a good idea to have a single
function for all the PGDATA directories, I'm not sure if one would
ever need the info like type, change, creation path etc. If we do
this, the function will become the linux equivalent command. I don't
see the difference between modification and change time stamps. For
debugging or analytical purposes in production environments, one would
majorly look at the file name, it's size on the disk, modification
time (to decide whether the file is stale or not, creation time (to
decide how old is the file), file/directory(maybe?). I'm not sure if
your patch has a recursive option for pg_ls_dir_metadata(), if it has,
I think it's more complex from a usability perspective.

And the functions like pg_ls_tmpdir, pg_ls_tmpdir, pg_ls_waldir etc.
(existing) and pg_ls_logicaldir, pg_ls_replslotdir (yet to have) will
provide the better usability compared to a generic function. Having
said this, I don't oppose having a generic function returning the file
name, file size, modification time, creation time, but not other info,
please. If one is interested in knowing the other information file
type, path etc. they can go run linux/windows/OS commands.

In summary what I think at this point is:
1) pg_ls_logicaldir, pg_ls_replslotdir - better for usability and
serving special purpose like their peers
2) modify pg_ls_dir such that it returns the file name, file size,
modification time, creation time, for directories, to be simple, it
shouldn't go recursively over all the directories, it should just
return the directory name, size, modification time, creation time.

If okay, I'm ready to spend time implementing them.

Thoughts?

Regards,
Bharath Rupireddy.

On Sat, Oct 23, 2021 at 11:10 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Oct 22, 2021 at 9:26 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

I concluded that it's better to add a function to list metadata of an arbitrary
dir, rather than adding more functions to handle specific, hardcoded dirs:
/messages/by-id/20191227170220.GE12890@telsasoft.com

I just had a quick look at the pg_ls_dir_metadata() patch(I didn't
look at the other patches). While it's a good idea to have a single
function for all the PGDATA directories, I'm not sure if one would
ever need the info like type, change, creation path etc. If we do
this, the function will become the linux equivalent command. I don't
see the difference between modification and change time stamps. For
debugging or analytical purposes in production environments, one would
majorly look at the file name, it's size on the disk, modification
time (to decide whether the file is stale or not, creation time (to
decide how old is the file), file/directory(maybe?). I'm not sure if
your patch has a recursive option for pg_ls_dir_metadata(), if it has,
I think it's more complex from a usability perspective.

And the functions like pg_ls_tmpdir, pg_ls_tmpdir, pg_ls_waldir etc.
(existing) and pg_ls_logicaldir, pg_ls_replslotdir (yet to have) will
provide the better usability compared to a generic function. Having
said this, I don't oppose having a generic function returning the file
name, file size, modification time, creation time, but not other info,
please. If one is interested in knowing the other information file
type, path etc. they can go run linux/windows/OS commands.

In summary what I think at this point is:
1) pg_ls_logicaldir, pg_ls_replslotdir - better for usability and
serving special purpose like their peers

I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
pg_ls_replslotdir, and attached the patch. The sample output looks
like [1]postgres=# select pg_ls_logicalsnapdir(); pg_ls_logicalsnapdir ----------------------------------------------- (0-14A50C0.snap,128,"2021-10-30 09:15:56+00") (0-14C46D8.snap,128,"2021-10-30 09:16:05+00") (0-14C97C8.snap,132,"2021-10-30 09:16:20+00"). Please review it further.

Here's the CF entry - https://commitfest.postgresql.org/35/3390/

[1]: postgres=# select pg_ls_logicalsnapdir(); pg_ls_logicalsnapdir ----------------------------------------------- (0-14A50C0.snap,128,"2021-10-30 09:15:56+00") (0-14C46D8.snap,128,"2021-10-30 09:16:05+00") (0-14C97C8.snap,132,"2021-10-30 09:16:20+00")
postgres=# select pg_ls_logicalsnapdir();
pg_ls_logicalsnapdir
-----------------------------------------------
(0-14A50C0.snap,128,"2021-10-30 09:15:56+00")
(0-14C46D8.snap,128,"2021-10-30 09:16:05+00")
(0-14C97C8.snap,132,"2021-10-30 09:16:20+00")

postgres=# select pg_ls_logicalmapdir();
pg_ls_logicalmapdir
---------------------------------------------------------------
(map-31d5-4eb-0_CDDDE88-2d9-2db,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CDDDE88-2da-2db,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE48038-2dc-2de,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE6BAF0-2dd-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CD97DE0-2d9-2d9,36,"2021-10-30 09:24:30+00")
(map-31d5-4eb-0_CE24808-2da-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE01200-2dc-2dc,36,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CDDDE88-2db-2db,36,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE6BAF0-2dc-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CDBA920-2d9-2da,108,"2021-10-30 09:24:32+00")
(map-31d5-4eb-0_CE01200-2da-2dc,108,"2021-10-30 09:24:34+00")
(map-31d5-4eb-0_CE6BAF0-2d9-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2db-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE6BAF0-2db-2df,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2dd-2dd,36,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CE24808-2dc-2dd,108,"2021-10-30 09:24:35+00")
(map-31d5-4eb-0_CD74E48-2d8-2d8,36,"2021-10-30 09:24:25+00")
(map-31d5-4eb-0_CE24808-2d9-2dd,108,"2021-10-30 09:24:35+00")

postgres=# select pg_ls_replslotdir('mysub');
pg_ls_replslotdir
-----------------------------------------------------------------
(xid-722-lsn-0-2000000.spill,36592640,"2021-10-30 09:18:29+00")
(xid-722-lsn-0-5000000.spill,4577860,"2021-10-30 09:18:32+00")
(state,200,"2021-10-30 09:18:25+00")
(xid-722-lsn-0-1000000.spill,25644220,"2021-10-30 09:18:29+00")
(xid-722-lsn-0-4000000.spill,36592640,"2021-10-30 09:18:32+00")
(xid-722-lsn-0-3000000.spill,36592640,"2021-10-30 09:18:32+00")

Regards,
Bharath Rupireddy.

On 10/30/21, 2:36 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
pg_ls_replslotdir, and attached the patch. The sample output looks
like [1]. Please review it further.

I took a look at the patch.

+ char path[MAXPGPATH + 11];

Why are you adding 11 to MAXPGPATH here? I would think that MAXPGPATH
is sufficient.

+	filename = text_to_cstring(filename_t);
+	snprintf(path, sizeof(path), "%s/%s", "pg_replslot", filename);
+	return pg_ls_dir_files(fcinfo, path, false);

I think we need to do some additional input validation here. It's
pretty easy to use this to see the contents of other directories.

postgres=# SELECT * FROM pg_ls_replslotdir('../');
name | size | modification
----------------------+-------+------------------------
postgresql.conf | 28995 | 2021-11-17 18:40:33+00
pg_hba.conf | 4789 | 2021-11-17 18:40:33+00
postmaster.opts | 39 | 2021-11-17 18:43:07+00
postgresql.auto.conf | 88 | 2021-11-17 18:40:33+00
pg_ident.conf | 1636 | 2021-11-17 18:40:33+00
postmaster.pid | 95 | 2021-11-17 18:43:07+00
PG_VERSION | 3 | 2021-11-17 18:40:33+00
(7 rows)

Nathan

On Wed, Nov 17, 2021 at 06:46:47PM +0000, Bossart, Nathan wrote:

On 10/30/21, 2:36 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
pg_ls_replslotdir, and attached the patch. The sample output looks
like [1]. Please review it further.

I took a look at the patch.

+	char		path[MAXPGPATH + 11];
+	filename = text_to_cstring(filename_t);
+	snprintf(path, sizeof(path), "%s/%s", "pg_replslot", filename);
+	return pg_ls_dir_files(fcinfo, path, false);

Why are you adding 11 to MAXPGPATH here? I would think that MAXPGPATH
is sufficient.

I suppose it's for "pg_replslot" - but it forgot about the "/".

MAXPGPATH isn't sufficient (even if you add 12), since it's a user-supplied
string. snprintf keeps it from overflowing the buffer, but its return value
isn't checked, so it could (hypothetically) return a result for the wrong slot,
if the slot name were very long, or MAXPGPATH were very short..

+ text *filename_t = PG_GETARG_TEXT_PP(0);

I think we need to do some additional input validation here. It's
pretty easy to use this to see the contents of other directories.

Actually, limiting the dir seems like a valid reason to add this function,
since it would allow GRANTing privileges for just those directories.

So now I agree that this patch should be included. But I suggest to add it
after my "ls" patches, which change the output fields, and include directories.
Directories might normally not be present, but an extension might put them
there. (And it may be important to show things that aren't supposed to be
there, too).

--
Justin

On Thu, Nov 18, 2021 at 12:16 AM Bossart, Nathan <bossartn@amazon.com> wrote:

On 10/30/21, 2:36 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
pg_ls_replslotdir, and attached the patch. The sample output looks
like [1]. Please review it further.

I took a look at the patch.

+ char path[MAXPGPATH + 11];

Why are you adding 11 to MAXPGPATH here? I would think that MAXPGPATH
is sufficient.

Yeah, MAXPGPATH is sufficient. Note that the replication slot name be
at max NAMEDATALEN(64 bytes) size
(ReplicationSlotPersistentData->name) and what we pass to the
pg_ls_dir_files is
"pg_replslot/<<user_entered_slot_name_with_max_64_bytes>>", so it can
never cross MAXPGPATH (1024).

+       filename = text_to_cstring(filename_t);
+       snprintf(path, sizeof(path), "%s/%s", "pg_replslot", filename);
+       return pg_ls_dir_files(fcinfo, path, false);

I think we need to do some additional input validation here. It's
pretty easy to use this to see the contents of other directories.

Done. Checking if the entered slot exists or not, if not throwing an
error, see [1]postgres=# select * from pg_ls_replslotdir(''); ERROR: replication slot "" does not exist postgres=# select * from pg_ls_replslotdir('../'); ERROR: replication slot "../" does not exist postgres=# select pg_ls_replslotdir('mysub'); pg_ls_replslotdir ----------------------------------------------------------------- (xid-722-lsn-0-2000000.spill,36592640,"2021-11-18 07:34:40+00") (xid-722-lsn-0-5000000.spill,36592640,"2021-11-18 07:34:43+00") (xid-722-lsn-0-A000000.spill,29910720,"2021-11-18 07:34:48+00") (xid-722-lsn-0-7000000.spill,36592640,"2021-11-18 07:34:45+00") (xid-722-lsn-0-9000000.spill,36592640,"2021-11-18 07:34:47+00") (state,200,"2021-11-18 07:34:36+00") (xid-722-lsn-0-8000000.spill,36592500,"2021-11-18 07:34:46+00") (xid-722-lsn-0-6000000.spill,36592640,"2021-11-18 07:34:44+00") (xid-722-lsn-0-1000000.spill,11171300,"2021-11-18 07:34:39+00") (xid-722-lsn-0-4000000.spill,36592500,"2021-11-18 07:34:42+00") (xid-722-lsn-0-3000000.spill,36592640,"2021-11-18 07:34:42+00") (11 rows).

Please review the attached v2.

[1]: postgres=# select * from pg_ls_replslotdir(''); ERROR: replication slot "" does not exist postgres=# select * from pg_ls_replslotdir('../'); ERROR: replication slot "../" does not exist postgres=# select pg_ls_replslotdir('mysub'); pg_ls_replslotdir ----------------------------------------------------------------- (xid-722-lsn-0-2000000.spill,36592640,"2021-11-18 07:34:40+00") (xid-722-lsn-0-5000000.spill,36592640,"2021-11-18 07:34:43+00") (xid-722-lsn-0-A000000.spill,29910720,"2021-11-18 07:34:48+00") (xid-722-lsn-0-7000000.spill,36592640,"2021-11-18 07:34:45+00") (xid-722-lsn-0-9000000.spill,36592640,"2021-11-18 07:34:47+00") (state,200,"2021-11-18 07:34:36+00") (xid-722-lsn-0-8000000.spill,36592500,"2021-11-18 07:34:46+00") (xid-722-lsn-0-6000000.spill,36592640,"2021-11-18 07:34:44+00") (xid-722-lsn-0-1000000.spill,11171300,"2021-11-18 07:34:39+00") (xid-722-lsn-0-4000000.spill,36592500,"2021-11-18 07:34:42+00") (xid-722-lsn-0-3000000.spill,36592640,"2021-11-18 07:34:42+00") (11 rows)
postgres=# select * from pg_ls_replslotdir('');
ERROR: replication slot "" does not exist
postgres=# select * from pg_ls_replslotdir('../');
ERROR: replication slot "../" does not exist
postgres=# select pg_ls_replslotdir('mysub');
pg_ls_replslotdir
-----------------------------------------------------------------
(xid-722-lsn-0-2000000.spill,36592640,"2021-11-18 07:34:40+00")
(xid-722-lsn-0-5000000.spill,36592640,"2021-11-18 07:34:43+00")
(xid-722-lsn-0-A000000.spill,29910720,"2021-11-18 07:34:48+00")
(xid-722-lsn-0-7000000.spill,36592640,"2021-11-18 07:34:45+00")
(xid-722-lsn-0-9000000.spill,36592640,"2021-11-18 07:34:47+00")
(state,200,"2021-11-18 07:34:36+00")
(xid-722-lsn-0-8000000.spill,36592500,"2021-11-18 07:34:46+00")
(xid-722-lsn-0-6000000.spill,36592640,"2021-11-18 07:34:44+00")
(xid-722-lsn-0-1000000.spill,11171300,"2021-11-18 07:34:39+00")
(xid-722-lsn-0-4000000.spill,36592500,"2021-11-18 07:34:42+00")
(xid-722-lsn-0-3000000.spill,36592640,"2021-11-18 07:34:42+00")
(11 rows)

Regards,
Bharath Rupireddy.

On 11/17/21, 11:39 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Please review the attached v2.

LGTM. I've marked this one as ready-for-committer.

Nathan

On Sat, Nov 20, 2021 at 12:29:51AM +0000, Bossart, Nathan wrote:

On 11/17/21, 11:39 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Please review the attached v2.

LGTM. I've marked this one as ready-for-committer.

One issue that I have with this patch is that there are zero
regression tests. Could you add a couple of things in
misc_functions.sql (for the negative tests perhaps) or
contrib/test_decoding/, taking advantage of places where slots are
already created? You may want to look after the non-superuser case
where the calls should fail, and the second case where a role is part
of pg_monitor where the call succeeds. Note that any roles created in
the tests have to be prefixed with "regress_".

+   snprintf(path, sizeof(path), "%s/%s", "pg_replslot", slotname);
+   return pg_ls_dir_files(fcinfo, path, false);
"pg_replslot" could be part of the third argument here.  There is no
need to separate it.

+        ordinary file in the server's pg_logical/mappings directory.
Paths had better have <filename> markups around them, no?
--
Michael

On Sun, Nov 21, 2021 at 6:58 AM Michael Paquier <michael@paquier.xyz> wrote:

On Sat, Nov 20, 2021 at 12:29:51AM +0000, Bossart, Nathan wrote:

On 11/17/21, 11:39 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Please review the attached v2.

LGTM. I've marked this one as ready-for-committer.

One issue that I have with this patch is that there are zero
regression tests. Could you add a couple of things in
misc_functions.sql (for the negative tests perhaps) or
contrib/test_decoding/, taking advantage of places where slots are
already created? You may want to look after the non-superuser case
where the calls should fail, and the second case where a role is part
of pg_monitor where the call succeeds. Note that any roles created in
the tests have to be prefixed with "regress_".

I don't think we need to go far to contrib/test_decoding/, even if we
add it there we can't test it for the outputs of these functions, so
I've added the tests in misc_functinos.sql itself.

+   snprintf(path, sizeof(path), "%s/%s", "pg_replslot", slotname);
+   return pg_ls_dir_files(fcinfo, path, false);
"pg_replslot" could be part of the third argument here.  There is no
need to separate it.

Done.

+ ordinary file in the server's pg_logical/mappings directory.
Paths had better have <filename> markups around them, no?

Done.

Attached v3 patch, please review it further.

Regards,
Bharath Rupireddy.

On Sun, Nov 21, 2021 at 08:45:52AM +0530, Bharath Rupireddy wrote:

I don't think we need to go far to contrib/test_decoding/, even if we
add it there we can't test it for the outputs of these functions, so
I've added the tests in misc_functinos.sql itself.

+SELECT COUNT(*) >= 0 AS OK FROM pg_ls_replslotdir('slot_dir_funcs');
+ ok
+----
+ t
+(1 row)
Creating a slot within the main regression test suite is something we
should avoid as it impacts the portability of the tests (note that we
don't have tests creating slots in src/test/regress/, and we'd require
max_replication_slots > 0 with this version of the patch).  This was
the point I was trying to make upthread about using test_decoding/
where we already have slots.

A second thing I have noticed is the set of OIDs used by the patch
which was incorrect. On a development branch, we require new features
to use OIDs between 8000-9999 (unused_oids would recommend a random
range of them). A third thing was that pg_proc.dat had an incorrect
description for pg_ls_replslotdir(), and that it was in need of
indentation.

I have tweaked a bit the tests and the docs, and the result looked
fine at the end. Hence, applied.
--
Michael