On Wed, Oct 11, 2023 at 4:27 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > Thank you for reviewing! PSA new version. >
Some more comments: 1. Let's restruture binary_upgrade_validate_wal_logical_end() a bit. First, let's change its name to binary_upgrade_slot_has_pending_wal() or something like that. Then move the context creation and free related code into DecodingContextHasDecodedItems(). We can rename DecodingContextHasDecodedItems() as pg_logical_replication_slot_has_pending_wal() and place it in slotfuncs.c. This will make the code structure similar to other slot functions like pg_replication_slot_advance(). 2. + * Returns true if there are no changes after the confirmed_flush_lsn. How about something like: "Returns true if there are no decodable WAL records after the confirmed_flush_lsn."? 3. Shouldn't we need to call CheckSlotPermissions() in binary_upgrade_validate_wal_logical_end? 4. + /* + * Also, set processing_required flag if the message is not + * transactional. It is needed to notify the message's existence to + * the caller side. Usually, the flag is set when either the COMMIT or + * ABORT records are decoded, but this must be turned on here because + * the non-transactional logical message is decoded without waiting + * for these records. + */ The first sentence of the comments doesn't seem to be required as that just says what the code does. So, let's slightly change it to: "We need to set processing_required flag to notify the message's existence to the caller side. Usually, the flag is set when either the COMMIT or ABORT records are decoded, but this must be turned on here because the non-transactional logical message is decoded without waiting for these records." -- With Regards, Amit Kapila.
