On Tue, Aug 23, 2022 at 11:37 PM Nathan Bossart <nathandboss...@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/
