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

On Tue, Mar 9, 2021 at 3:22 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>

Few comments:
==================

1.
+/*
+ * Handle the PREPARE spoolfile (if any)
+ *
+ * It can be necessary to redirect the PREPARE messages to a spoolfile (see
+ * apply_handle_begin_prepare) and then replay them back at the COMMIT PREPARED
+ * time. If needed, this is the common function to do that file redirection.
+ *

I think the last sentence ("If needed, this is the ..." in the above
comments is not required.

2.
+prepare_spoolfile_exists(char *path)
+{
+ bool found;
+
+ File fd = PathNameOpenFile(path, O_RDONLY | PG_BINARY);
+
+ found = fd >= 0;
+ if (fd >= 0)
+ FileClose(fd);

Can we avoid using bool variable in the above code with something like below?

File fd = PathNameOpenFile(path, O_RDONLY | PG_BINARY);

if (fd >= 0)
{
FileClose(fd);
return true;
}

return false;

3. In prepare_spoolfile_replay_messages(), it is better to free the
memory allocated for temporary strings buffer and s2.

4.
+ /* check if the file already exists. */
+ file_found = prepare_spoolfile_exists(path);
+
+ if (!file_found)
+ {
+ elog(DEBUG1, "Not found file \"%s\". Create it.", path);
+ psf_cur.vfd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
+ if (psf_cur.vfd < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not create file \"%s\": %m", path)));
+ }
+ else
+ {
+ /*
+ * Open the file and seek to the beginning because we always want to
+ * create/overwrite this file.
+ */
+ elog(DEBUG1, "Found file \"%s\". Overwrite it.", path);
+ psf_cur.vfd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_TRUNC | PG_BINARY);
+ if (psf_cur.vfd < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m", path)));
+ }

Here, whether the file exists or not you are using the same flags to
open it which seems correct to me but the code looks a bit odd. Why do
we in this case even bother to check if it exists? Is it for DEBUG
message, if so not sure if that is worth it? I am also thinking why
not have a function prepare_spoolfile_open similar to *_close and call
it from all the places with the mode where you can indicate whether
you want to create or open the file.

5. I think prepare_spoolfile_close can be extended to take PsfFile as
input and then it can be also used from
prepare_spoolfile_replay_messages.

6. I think we should also write some commentary about prepared
transactions atop of worker.c as we have done for streamed
transactions.

--
With Regards,
Amit Kapila.