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

On Mon, Jul 20, 2020 at 12:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 4:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 12:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Wed, Jul 15, 2020 at 6:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > Let me know what you think of the changes?
> > >
> > > I have reviewed the changes and looks fine to me.
> > >
> >
> > Thanks, I am planning to start committing a few of the infrastructure
> > patches (especially first two) by early next week as we have resolved
> > all the open issues and done an extensive review of the entire
> > patch-set.  In the attached version, there is a slight change in one
> > of the commit messages as compared to the previous version.  I would
> > like to describe in brief the first two patches for the sake of
> > convenience.  Let me know if you or anyone else sees any problems with
> > these.
> >
> > The first patch in the series allows us to WAL-log subtransaction and
> > top-level XID association.  The logical decoding infrastructure needs
> > to know which top-level
> > transaction the subxact belongs to, in order to decode all the
> > changes. Until now that might be delayed until commit, due to the
> > caching (GPROC_MAX_CACHED_SUBXIDS), preventing features requiring
> > incremental decoding.  So we also write the assignment info into WAL
> > immediately, as part of the next WAL record (to minimize overhead)
> > only when *wal_level=logical*.  We can not remove the existing
> > XLOG_XACT_ASSIGNMENT WAL as that is required for avoiding overflow in
> > the hot standby snapshot.
> >

Pushed, this patch.

> >
>
> The patch set required to rebase after committing the binary format
> option support in the create subscription command.  I have rebased the
> patch set on the latest head and also added a test case to test
> streaming in binary format.
>

While going through commit 9de77b5453, I noticed below change:

@@ -424,6 +424,10 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
        PQfreemem(pubnames_literal);
        pfree(pubnames_str);

+       if (options->proto.logical.binary &&
+           PQserverVersion(conn->streamConn) >= 140000)
+           appendStringInfoString(&cmd, ", binary 'true'");
+

Now, the similar change in this patch series is as below:

@@ -408,6 +408,9 @@ libpqrcv_startstreaming(WalReceiverConn *conn,
  appendStringInfo(&cmd, "proto_version '%u'",
  options->proto.logical.proto_version);

+ if (options->proto.logical.streaming)
+ appendStringInfo(&cmd, ", streaming 'on'");
+

I think we also need a version check similar to commit 9de77b5453 to
ensure that we send the new option only when connected to a newer
version (>=14) primary server.

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