On Tue, Aug 23, 2022 at 11:37 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> On Wed, Aug 17, 2022 at 12:39:26PM +0530, Bharath Rupireddy wrote:
> > + /*
> > + * We're only handling directories here, skip if it's not ours. Also, skip
> > + * if the caller has already performed this check.
> > + */
> > + if (!slot_dir_checked &&
> > + lstat(path, &statbuf) == 0 && !S_ISDIR(statbuf.st_mode))
> > return;
>
> There was previous discussion about removing this stanza from
> ReorderBufferCleanupSeralizedTXNs() completely [0]. If that is still
> possible, I think I would choose that over having callers specify whether
> to do the directory check.
>
> [0] https://postgr.es/m/20220329224832.GA560657%40nathanxps13
From [0]:
> My guess is that this was done because ReorderBufferCleanupSerializedTXNs()
> is also called from ReorderBufferAllocate() and ReorderBufferFree().
> However, it is odd that we just silently return if the slot path isn't a
> directory in those cases. I think we could use get_dirent_type() in
> StartupReorderBuffer() as you suggested, and then we could let ReadDir()
> ERROR for non-directories for the other callers of
> ReorderBufferCleanupSerializedTXNs(). WDYT?
Firstly, removing lstat() completely from
ReorderBufferCleanupSerializedTXNs() is a behavioural change.
ReorderBufferCleanupSerializedTXNs() returning silently to callers
ReorderBufferAllocate() and ReorderBufferFree(), when the slot path
isn't a directory completely makes sense to me because the callers are
trying to clean something and if it doesn't exist that's okay, they
can go ahead. Also, the usage of the ReorderBufferAllocate() and
ReorderBufferFree() is pretty wide and I don't want to risk the new
behaviour.
> > + /* we're only handling directories here, skip if it's not one */
> > + sprintf(path, "pg_replslot/%s", logical_de->d_name);
> > + if (get_dirent_type(path, logical_de, false, LOG) != PGFILETYPE_DIR)
> > + continue;
>
> IIUC an error in get_dirent_type() could cause slots to be skipped here,
> which is a behavior change.
That behaviour hasn't changed, no? Currently, if lstat() fails in
ReorderBufferCleanupSerializedTXNs() it returns to
StartupReorderBuffer()'s while loop which is in a way continuing with
the other slots, this patch does nothing new.
So, no new patch, please find the latest v16 patch at [1].
[1] https://www.postgresql.org/message-id/CALj2ACXZPMYXrPZ%2BCUE0_5DKDnxyqDGRftSgPPx--PWhWB3RtQ%40mail.gmail.com
--
Bharath Rupireddy
RDS Open Source Databases: https://aws.amazon.com/rds/postgresql/