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

On Fri, May 1, 2020 at 8:41 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Apr 30, 2020 at 12:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?
b. There is an extra space between detailed and comments. A similar
change is required at other place where this comment is used.
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.

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."

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?

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."

> Apart from this, I have also fixed one defect raised by my colleague
> Neha Sharma. That issue is the incomplete toast tuple flag was not
> reset when the main table tuple was inserted through speculative
> insert and due to that data was not streamed even if later we were
> getting speculative confirm because incomplete toast flag was never
> reset. This patch also includes the fix for the issue raised by Erik.
>

It would be better if you can mention which all patches contain the
changes as it will be easier to review the fix.

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