Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Erik Rijkers <er@xs4all.nl>, Kuntal Ghosh <kuntalghosh.2007@gmail.com>, Tomas Vondra <tomas.vondra@2ndquadrant.com>, Michael Paquier <michael@paquier.xyz>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2020-06-15T12:59:54Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Tighten the concurrent abort check during decoding.

  2. Improve hash_create()'s API for some added robustness.

  3. Use HASH_BLOBS for xidhash.

  4. Fix initialization of RelationSyncEntry for streaming transactions.

  5. Remove unused function declaration in logicalproto.h.

  6. Add additional tests to test streaming of in-progress transactions.

  7. Fix inline marking introduced in commit 464824323e.

  8. Add support for streaming to built-in logical replication.

  9. Fix the SharedFileSetUnregister API.

  10. Fix comment in procarray.c

  11. Suppress compiler warning in non-cassert builds.

  12. Extend the BufFile interface.

  13. Mark a few logical decoding related variables with PGDLLIMPORT.

  14. Implement streaming mode in ReorderBuffer.

  15. Extend the logical decoding output plugin API with stream methods.

  16. WAL Log invalidations at command end with wal_level=logical.

  17. Immediately WAL-log subtransaction and top-level XID association.

  18. Allow logical replication to transfer data in binary format.

  19. Only superuser can set sslcert/sslkey in postgres_fdw user mappings

  20. Track statistics for spilling of changes from ReorderBuffer.

  21. Add logical_decoding_work_mem to limit ReorderBuffer memory usage.

  22. logical decoding: process ASSIGNMENT during snapshot build

  23. Emit invalidations to standby for transactions without xid.

On Mon, Jun 15, 2020 at 9:12 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Jun 12, 2020 at 4:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> > >  Basically, this part is still
> > > I have to work upon, once we get the consensus then I can remove those
> > > extra wait event from the patch.
> > >
> >
> > Okay, feel free to send an updated patch with the above change.
>
> Sure, I will do that in the next patch set.
>

I have few more comments on the patch
0013-Change-buffile-interface-required-for-streaming-.patch:

1.
- * temp_file_limit of the caller, are read-only and are automatically closed
- * at the end of the transaction but are not deleted on close.
+ * temp_file_limit of the caller, are read-only if the flag is set and are
+ * automatically closed at the end of the transaction but are not deleted on
+ * close.
  */
 File
-PathNameOpenTemporaryFile(const char *path)
+PathNameOpenTemporaryFile(const char *path, int mode)

No need to say "are read-only if the flag is set".  I don't see any
flag passed to function so that part of the comment doesn't seem
appropriate.

2.
@@ -68,7 +68,8 @@ SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg)
  }

  /* Register our cleanup callback. */
- on_dsm_detach(seg, SharedFileSetOnDetach, PointerGetDatum(fileset));
+ if (seg)
+ on_dsm_detach(seg, SharedFileSetOnDetach, PointerGetDatum(fileset));
 }

Add comments atop function to explain when we don't want to register
the dsm detach stuff?

3.
+ */
+ newFile = file->numFiles - 1;
+ newOffset = FileSize(file->files[file->numFiles - 1]);
  break;

FileSize can return negative lengths to indicate failure which we
should handle.  See other places in the code where FileSize is used?
But I have another question here which is why we need to implement
SEEK_END?  How other usages of BufFile interface takes care of this?
I see an API BufFileTell which can give the current read/write
location in the file, isn't that sufficient for your usage?  Also, how
before BufFile usage is this thing handled in the patch?

4.
+ /* Loop over all the  files upto the fileno which we want to truncate. */
+ for (i = file->numFiles - 1; i >= fileno; i--)

"the  files", extra space in the above part of the comment.

5.
+ /*
+ * Except the fileno,  we can directly delete other files.

Before 'we', there is extra space.

6.
+ else
+ {
+ FileTruncate(file->files[i], offset, WAIT_EVENT_BUFFILE_READ);
+ newOffset = offset;
+ }

The wait event passed here doesn't seem to be appropriate.  You might
want to introduce a new wait event WAIT_EVENT_BUFFILE_TRUNCATE.  Also,
the error handling for FileTruncate is missing.

7.
+ if ((i != fileno || offset == 0) && fileno != 0)
+ {
+ SharedSegmentName(segment_name, file->name, i);
+ SharedFileSetDelete(file->fileset, segment_name, true);
+ newFile--;
+ newOffset = MAX_PHYSICAL_FILESIZE;
+ }

Similar to the previous comment, I think we should handle the failure
of SharedFileSetDelete.

8. I think the comments related to BufFile shared API usage need to be
expanded in the code to explain the new usage.  For ex., see the below
comments atop buffile.c
* BufFile supports temporary files that can be made read-only and shared with
* other backends, as infrastructure for parallel execution.  Such files need
* to be created as a member of a SharedFileSet that all participants are
* attached to.


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