Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Fri, May 22, 2020 at 11:54 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> v22-0006-Add-support-for-streaming-to-built-in-replicatio
> ----------------------------------------------------------------------------
>
Few more comments on v22-0006 patch:

1.
+stream_cleanup_files(Oid subid, TransactionId xid, bool missing_ok)
+{
+ int i;
+ char path[MAXPGPATH];
+ bool found = false;
+
+ subxact_filename(path, subid, xid);
+
+ if ((unlink(path) < 0) && (errno != ENOENT) && !missing_ok)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));

Here, we have unlinked the files containing information of subxacts
but don't we need to free the corresponding memory (memory for
subxacts) as well?

2.
apply_handle_stream_abort()
{
..
+ subxact_filename(path, MyLogicalRepWorker->subid, xid);
+
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+
+ return;
..
}

Like the previous comment, it seems here also we need to free subxacts
memory and additionally we forgot to adjust the xids array as well.

3.
apply_handle_stream_abort()
{
..
+ /* XXX optimize the search by bsearch on sorted data */
+ for (i = nsubxacts; i > 0; i--)
+ {
+ if (subxacts[i - 1].xid == subxid)
+ {
+ subidx = (i - 1);
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return;
..
}

Is it possible that we didn't find the xid in subxacts array? If so,
I think we should mention the same in comments, otherwise, we should
have an assert for found.

4.
apply_handle_stream_abort()
{
..
+ changes_filename(path, MyLogicalRepWorker->subid, xid);
+
+ if (truncate(path, subxacts[subidx].offset))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not truncate file \"%s\": %m", path)));
..
}

Will truncate works on Windows? I see in the code we ftruncate which
is defined as chsize in win32.h and win32_port.h. I have not tested
this so I am not very sure about this. I got a below warning when I
tried to compile this code on Windows. I think it is better to
ftruncate as it is used at other places in the code as well.

worker.c(798): warning C4013: 'truncate' undefined; assuming extern
returning int

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com