On Tue, Apr 28, 2020 at 3:11 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Apr 27, 2020 at 4:05 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > [latest patches] > > v16-0004-Gracefully-handle-concurrent-aborts-of-uncommitt > - Any actions leading to transaction ID assignment are prohibited. > That, among others, > + 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. > + Access via the <literal>heap_*</literal> scan APIs will error out. > + Additionally, any actions leading to transaction ID assignment > are prohibited. That, among others, > .. > @@ -1383,6 +1392,14 @@ heap_fetch(Relation relation, > bool valid; > > /* > + * We don't expect direct calls to heap_fetch with valid > + * CheckXidAlive for regular tables. Track that below. > + */ > + if (unlikely(TransactionIdIsValid(CheckXidAlive) && > + !(IsCatalogRelation(relation) || RelationIsUsedAsCatalogTable(relation)))) > + elog(ERROR, "unexpected heap_fetch call during logical decoding"); > + > > I think comments and code don't match. In the comment, we are saying > that via output plugins access to user catalog tables or regular > system catalog tables won't be allowed via heap_* APIs but code > doesn't seem to reflect it. I feel only > TransactionIdIsValid(CheckXidAlive) is sufficient here. See, the > original discussion about this point [1] (Refer "I think it'd also be > good to add assertions to codepaths not going through systable_* > asserting that ...").
Right, So I think we can just add an assert in these function that Assert(!TransactionIdIsValid(CheckXidAlive)) ? > > Isn't it better to block the scan to user catalog tables or regular > system catalog tables for tableam scan APIs rather than at the heap > level? There might be some APIs like heap_getnext where such a check > might still be required but I guess it is still better to block at > tableam level. > > [1] - > https://www.postgresql.org/message-id/20180726200241.aje4dv4jsv25v4k2%40alap3.anarazel.de Okay, let me analyze this part. Because someplace we have to keep at heap level like heap_getnext and other places at tableam level so it seems a bit inconsistent. Also, I think the number of checks might going to increase because some of the heap functions like heap_hot_search_buffer are being called from multiple tableam calls, so we need to put check at every place. Another point is that I feel some of the checks what we have today might not be required like heap_finish_speculative, is not fetching any tuple for us so why do we need to care about this function? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
