Dear Vignesh, Thank you for reviewing! PSA new version patch set.
> Few minor comments:
> 1) we could remove the variable slotname from the below code by using
> PQgetvalue directly in pg_log:
> + for (i = 0; i < ntups; i++)
> + {
> + char *slotname;
> +
> + is_error = true;
> +
> + slotname = PQgetvalue(res, i, i_slotname);
> +
> + pg_log(PG_WARNING,
> + "\nWARNING: logical replication slot \"%s\"
> is not active",
> + slotname);
> + }
Removed. Such codes were in two functions, and both of them were fixed.
> 2) This include "catalog/pg_control.h" should be after inclusion
> pg_collation.h
> #include "catalog/pg_authid_d.h"
> +#include "catalog/pg_control.h"
> #include "catalog/pg_collation.h"
Moved.
> 3) This spurious addition line change might not be required in this patch:
> --- a/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
> +++ b/src/bin/pg_upgrade/t/003_logical_replication_slots.pl
> @@ -85,11 +85,39 @@ $old_node->safe_psql(
> ]);
>
> my $result = $old_node->safe_psql('postgres',
> - "SELECT count (*) FROM
> pg_logical_slot_get_changes('test_slot', NULL, NULL)"
> + "SELECT count(*) FROM
> pg_logical_slot_peek_changes('test_slot', NULL, NULL)"
> );
> +
> is($result, qq(12), 'ensure WALs are not consumed yet');
> $old_node->stop;
I removed the line.
In the first place, what I wanted to check here was that pg_upgrade failed
because
WALs were not consumed. So if pg_logical_slot_get_changes() was called here, all
of WALs were consumed here and the subsequent command was sucseeded. This was
not
happy for us and that's why changed to pg_logical_slot_peek_changes().
But after considering more, I thought that calling the function was not the
mandatory
because no one needed the output.So removed.
> 4) This inclusion "#include "access/xlogrecord.h" is not required:
> #include "postgres_fe.h"
>
> +#include "access/xlogrecord.h"
> +#include "access/xlog_internal.h"
> #include "catalog/pg_authid_d.h"
Removed.
> 5)"thepublisher's" should be "the publisher's"
> When a live check is requested, there is a possibility of additional changes
> occurring, which may cause the current WAL position to exceed the
> confirmed_flush_lsn
> of the slot. As a result, we check the confirmed_flush_lsn of each logical
> slot
> instead. This is sufficient as all the WAL records will be sent during
> thepublisher's
> shutdown.
Fixed.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
v16-0001-pg_upgrade-Add-include-logical-replication-slots.patch
Description: v16-0001-pg_upgrade-Add-include-logical-replication-slots.patch
v16-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description: v16-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
v16-0003-pg_upgrade-Add-check-function-for-include-logica.patch
Description: v16-0003-pg_upgrade-Add-check-function-for-include-logica.patch
v16-0004-Change-the-method-used-to-check-logical-replicat.patch
Description: v16-0004-Change-the-method-used-to-check-logical-replicat.patch
