On 11/24/19 10:39 AM, Tom Lane wrote:
> Mark Dilger <hornschnorter@gmail.com> writes:
>> On 11/23/19 8:50 PM, Mark Dilger wrote:
>>> I have finished reading and applying your three patches and have moved
>>> on to testing them. I hope to finish the review soon.
>
>> After applying all three patches, the stress test that originally
>> uncovered the assert in predicate.c no longer triggers any asserts.
>> `check-world` passes. The code and comments look good.
>
> Thanks for reviewing!
>
> After sleeping on it, I'm not really happy with what I did in
> PrepareTransaction (that is, invent a separate PrePrepare_Notify
> function). The idea was to keep that looking parallel to what
> CommitTransaction does, and preserve infrastructure against the
> day that somebody gets motivated to allow LISTEN or NOTIFY in
> a prepared transaction. But on second thought, what would surely
> happen when that feature gets added is just that AtPrepare_Notify
> would serialize the pending LISTEN/NOTIFY actions into the 2PC
> state file. There wouldn't be any need for PrePrepare_Notify,
> so there's no point in introducing that. I'll just move the
> comment saying that nothing has to happen at that point for NOTIFY.
I looked at that. I thought it was an interesting decision to
factor out that error to its own function while leaving a
similar error inline just a little below in xact.c:
if ((MyXactFlags & XACT_FLAGS_ACCESSEDTEMPNAMESPACE))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot PREPARE a transaction that has operated
on temporary objects")));
I assumed you had factored it out in anticipation of supporting notify
here in the future. If you want to backtrack that decision and leave it
inline, you would still keep the test rather than just a comment, right?
It sounds like you intend to let AtPrepare_Notify catch the problem
later, but since that's just an Assert and not an ereport(ERROR), that
provides less error checking for non-assert builds.
--
Mark Dilger