On Sun, Mar 7, 2021 at 3:00 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Sun, Mar 7, 2021 at 7:35 AM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > Please find attached the latest patch set v51*
> >
>
> Few more comments on v51-0006-Fix-apply-worker-empty-prepare:
> ======================================================
> 1.
> +/*
> + * A Prepare spoolfile hash entry. We create this entry in the
> psf_hash. This is
> + * for maintaining a mapping between the name of the prepared
> spoolfile, and the
> + * corresponding fileset handles of same.
> + */
> +typedef struct PsfHashEntry
> +{
> + char name[MAXPGPATH]; /* Hash key --- must be first */
> + bool allow_delete; /* ok to delete? */
> +} PsfHashEntry;
> +
>
> IIUC, this has table is used for two purposes in the patch (a) to
> check for existence of prepare spool file where we anyway to check it
> on disk if not found in the hash table. (b) to allow the prepare spool
> file to be removed on proc_exit.
>
> I think we don't need the optimization provided by (a) because it will
> be too rare a case to deserve any optimization, we might write a
> comment in prepare_spoolfile_exists to indicate such an optimization.
> For (b), we can use a simple list to track files to be removed on
> proc_exit something like we do in CreateLockFile. I think avoiding
> hash table usage will reduce the code and chances of bugs in this
> area. It won't be easy to write a lot of automated tests to test this
> functionality so it is better to avoid minor optimizations at this
> stage.Our data structure psf_hash also needs to be able to discover the entry for a specific spool file and remove it. e.g. anything marked as "allow_delete = false" (during prepare) must be able to be re-found and removed from that structure at commit_prepared or rollback_prepared time. Looking at CreateLockFile code, I don't see that it is ever deleting entries from its "lock_files" list on-the-fly, so it's not really a fair comparison to say just use a List like CreateLockFile. So, even though we (currently) only have a single data member "allow_delete", I think the requirement to do a key lookup/delete makes a HTAB a more appropriate data structure than a List. ------ Kind Regards, Peter Smith. Fujitsu Australia
