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.
Regards,
Greg Nancarrow
Fujitsu Australia
