Re: Fix issuing of multiple command completions per command

Tom Lane wrote:
>
> Fernando Nasser <fnasser(at)redhat(dot)com> writes:
> > (Please note that "[PATCHES] Allow arbitrary levels of
> > analyze/rewriting" must be applied before this patch)
>
> Actually, it looks like you accidentally included that patch as part of
> this one. Since Bruce already applied that patch, this one will not
> apply cleanly.
>

Oops! I am attaching the corrected patch. Sorry.

> While this looks good as far as it goes, I don't think it
> resolves the problems exposed by Coax's recent complaint:
> http://archives.postgresql.org/pgsql-bugs/2002-02/msg00155.php
>

It would solve the completion tag part as it would send the INSERT.
Remember that you asked me to generate the tag based on the parse
tree (before the analyze/rewrite).

And I prevent a second command completion tag to clobber it.
We should ask him to try the patch. Coax says he is not very
concerned with the CommandInfo -- the application needs the
right CommandTag.

But yes, this patch is to fix the tag part. We can work together on
a follow-up patch to improve the CommandInfo part as well, but we
will have to change things around a bit, as you suggest.

> There are two remaining bogosities:
>
> 1. We need to get rid of the static variable CommandInfo that's
> manipulated by UpdateCommandInfo; static state is certainly going
> to be a bad idea in this context.
>

Humm, I believe it is only used from the Begin to the End calls.
It seems harmless to me, but it may indeed go away once
we rearrange things around to choose what is printed

> 2. EndCommand should *not* be issued until we have completed all the
> actions of the (rewritten) query. Moving EndCommand into
> PerformPortalFetch, as you did here, is exactly the wrong direction.
> What we need is for EndCommand to be executed from pg_exec_query_string.
> That means that the tag auxiliary info (currently set up by
> UpdateCommandInfo) has to be passed out from ProcessQuery to
> pg_exec_query_string, which is the only place that can know when to send
> the command completion notice back to the client.
>

That was my first impression, but after I looked more deeply into it
I figured that this is the right thing to do (I explain it below).

And remember that there is still the 'Z'. This completion will close
the
flow of data packets that may have been sent. It is good for the
async processing as well as the application may process the data
while waiting for the 'Z' (otherwise it will not know that all the
data has arrived).

> As the comments in dest.h and dest.c point out, BeginCommand/EndCommand
> are fairly confused in purpose and usage right now. Maybe rather than
> patching, we ought to decide what functionality we really need and do a
> major reconstruction job.
>

I think it was fine until rewriting of queries became more frequent.
It is actually still fine for plannable queries (with this patch
applied).

The way I see it is that this is a palnnable query thing where
Begin and End calls should open and close the flow of data.

For plannable queries we have:

BeginCommand()
<send data>
EndCommand()

It has to be properly adapted for utility queries and in that case
it has to be handled by pg_exec_query_string which is the one that
has a view of the utility processing as a whole (no sending of data
involved in this case either).

> One rather nasty issue that arises here is that I don't think
> pg_exec_query_string has enough knowledge to understand which of the
> queries produced by query rewriting corresponds to the "original" query.
> This is critical in order to figure out which command's auxiliary info
> is the right stuff to pass back to the client. I suspect we will need
> to expand QueryRewrite's API to identify the "main" query in the
> returned query list.
>

For tags we avoided this issue when you asked me to use the parse tree
to
generate the completion tokens. But I agree that we will probably have
to do exactly what you are saying if we want to control what is the
CommandInfo that will be printed with the CommandTag.

But consider also that the CommandInfo that corresponds to the data
stream was already sent. And it is zero anyway if more than one row
is affected, isn't it? Maybe it is not that broken after all
(with tis patch applied, I mean).

My suggestion is that we do this in stages. Get the tags fixed (this
patch), as CREATE SCHEMAS gets very weird without it. And that we
discuss
a little bit what to do with CommandInfo and create a patch for that.
Also, a single patch for both things will meddle with too much code.

--
Fernando Nasser
Red Hat Canada Ltd. E-Mail: fnasser(at)redhat(dot)com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9