Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Tue, May 5, 2020 at 4:06 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, May 4, 2020 at 5:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, May 1, 2020 at 8:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, Apr 30, 2020 at 12:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > But can't they access other catalogs like pg_publication*?  I think
> > > > the basic thing we want to ensure here is that all historic accesses
> > > > always use systable* APIs to access catalogs.  We can ensure that via
> > > > having Asserts (or elog(ERROR, ..) in heap/tableam APIs.
> > >
> > > Yeah, it can.  So I have changed it now, actually along with
> > > CheckXidLive, I have kept one more flag so whenever CheckXidLive is
> > > set and we pass through systable_beginscan we will set that flag.  So
> > > while accessing the tableam API we will set if CheckXidLive is set
> > > then another flag must also be set otherwise we through an error.
> > >
> >
> > Okay, I have reviewed these changes and below are my comments:
> >
> > Review of  v17-0004-Gracefully-handle-concurrent-aborts-of-uncommitt
> > --------------------------------------------------------------------
> > 1.
> > + /*
> > + * If CheckXidAlive is set then set a flag that this call is passed through
> > + * systable_beginscan.  See detailed  comments at snapmgr.c where these
> > + * variables are declared.
> > + */
> > + if (TransactionIdIsValid(CheckXidAlive))
> > + sysbegin_called = true;
> >
> > a. How about calling this variable as bsysscan or sysscan instead of
> > sysbegin_called?
>
> Done
>
> > b. There is an extra space between detailed and comments.  A similar
> > change is required at other place where this comment is used.
>
> Done
>
> > c. How about writing the first line as "If CheckXidAlive is set then
> > set a flag to indicate that system table scan is in-progress."
> >
> > 2.
> > -     Any actions leading to transaction ID assignment are prohibited.
> > That, among others,
> > -     includes writing to tables, performing DDL changes, and
> > -     calling <literal>pg_current_xact_id()</literal>.
> > +     Note that access to user catalog tables or regular system
> > catalog tables in
> > +     the output plugins has to be done via the
> > <literal>systable_*</literal> scan
> > +     APIs only. The user tables should not be accesed in the output
> > plugins anyways.
> > +     Access via the <literal>heap_*</literal> scan APIs will error out.
> >
> > The line "The user tables should not be accesed in the output plugins
> > anyways." seems a bit of out of place.  I don't think this is required
> > here.  If you read the previous paragraph in the same document it is
> > written: "Read only access to relations is permitted as long as only
> > relations are accessed that either have been created by
> > <command>initdb</command> in the <literal>pg_catalog</literal> schema,
> > or have been marked as user provided catalog tables using ...".  I
> > think that is sufficient to convey the information that the newly
> > added line by you is trying to convey.
>
> Right.
>
> >
> > 3.
> > + /*
> > + * We don't expect direct calls to this routine when CheckXidAlive is a
> > + * valid transaction id, this should only come through systable_* call.
> > + * CheckXidAlive is set during logical decoding of a transactions.
> > + */
> > + if (unlikely(TransactionIdIsValid(CheckXidAlive) && !sysbegin_called))
> > + elog(ERROR, "unexpected heap_getnext call during logical decoding");
> >
> > How about changing this comment as "We don't expect direct calls to
> > heap_getnext with valid CheckXidAlive for catalog or regular tables.
> > See detailed comments at snapmgr.c where these variables are
> > declared."?  Change the similar comment used in other places in the
> > patch.
> >
> > For this specific API, we can also say "Normally we have such a check
> > at tableam level API but this is called from many places so we need to
> > ensure it here."
>
> Done
>
> >
> > 4.
> > + * If CheckXidAlive is valid, then we check if it aborted. If it did, we error
> > + * out.  We can't directly use TransactionIdDidAbort as after crash such
> > + * transaction might not have been marked as aborted.  See detailed  comments
> > + * at snapmgr.c where the variable is declared.
> > + */
> > +static inline void
> > +HandleConcurrentAbort()
> >
> > Can we change the comments as "Error out, if CheckXidAlive is aborted.
> > We can't directly use TransactionIdDidAbort as after crash such
> > transaction might not have been marked as aborted."
> >
> > After this add one empty line and then we can say something like:
> > "This is a special API to check if CheckXidAlive is aborted in system
> > table scan APIs.  See detailed comments at snapmgr.c where the
> > variable is declared."
> >
> > 5. Shouldn't we add a check in table_scan_sample_next_block and
> > table_scan_sample_next_tuple APIs as well?
>
> Done
>
> > 6.
> > /*
> > + * An xid value pointing to a possibly ongoing (sub)transaction.
> > + * Currently used in logical decoding.  It's possible that such transactions
> > + * can get aborted while the decoding is ongoing.  If CheckXidAlive is set
> > + * then we will set sysbegin_called flag when we call systable_beginscan.  This
> > + * is to ensure that from the pgoutput plugin we should never directly access
> > + * the tableam or heap apis because we are checking for the concurrent abort
> > + * only in systable_* apis.
> > + */
> > +TransactionId CheckXidAlive = InvalidTransactionId;
> > +bool sysbegin_called = false;
> >
> > Can we change the above comment as "CheckXidAlive is a xid value
> > pointing to a possibly ongoing (sub)transaction.  Currently, it is
> > used in logical decoding.  It's possible that such transactions can
> > get aborted while the decoding is ongoing in which case we skip
> > decoding that particular transaction. To ensure that we check whether
> > the CheckXidAlive is aborted after fetching the tuple from system
> > tables.  We also ensure that during logical decoding we never directly
> > access the tableam or heap APIs because we are checking for the
> > concurrent aborts only in systable_* APIs."
>
> Done
>
> I have also fixed one issue in the patch
> v18-0010-Bugfix-handling-of-incomplete-toast-tuple.patch.
>
> Basically, the check, in ReorderBufferLargestTopTXN for selecting the
> largest top transaction was incorrect so I have fixed that.

There was one unrelated bug fix in v18-0010 patch reported by Neha
Sharma offlist so sending the updated version.



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com