On Fri, Feb 12, 2021 at 3:21 PM Zhihong Yu <zyu@yugabyte.com> wrote: > > Greg: > bq. we should just return parsetree->hasModifyingCTE at this point, > > Maybe you can clarify a bit. > The if (parsetree->hasModifyingCTE) check is followed by if (!hasModifyingCTE). > When parsetree->hasModifyingCTE is false, !hasModifyingCTE would be true, resulting in the execution of the if (!hasModifyingCTE) block. > > In your reply, did you mean that the if (!hasModifyingCTE) block is no longer needed ? (I guess not) >
Sorry for not making it clear. What I meant was that instead of:
if (parsetree->querySource == QSRC_ORIGINAL) { /* Assume original queries have hasModifyingCTE set correctly */ if (parsetree->hasModifyingCTE) hasModifyingCTE = true; }
I thought I should be able to use the following (it the setting for QSRC_ORIGINAL can really be trusted):
if (parsetree->querySource == QSRC_ORIGINAL) { /* Assume original queries have hasModifyingCTE set correctly */ return parsetree->hasModifyingCTE; }
(and then the "if (!hasModifyingCTE)" test on the code following immediately below it can be removed)
BUT - after testing that change, the problem test case (in the "with" tests) STILL fails. I then checked if hasModifyingCTE is always false in the QSRC_ORIGINAL case (by adding an Assert), and it always is false. So actually, there is no point in having the "if (parsetree->querySource == QSRC_ORIGINAL)" code block - even the so called "original" query doesn't maintain the setting correctly (even though the actual original query sent into the query rewriter does). And also then the "if (!hasModifyingCTE)" test on the code following immediately below it can be removed.
