Re: [HACKERS] logical decoding of two-phase transactions

On Sun, Mar 7, 2021 at 3:00 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Mar 7, 2021 at 7:35 AM Peter Smith <smithpb2250(at)gmail(dot)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