Add PortalDrop in exec_execute_message

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Add PortalDrop in exec_execute_message

Yura Sokolov
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Yura Sokolov
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Álvaro Herrera
On 2021-May-25, Yura Sokolov wrote:

> Tom Lane писал 2021-05-21 21:23:
> > Yura Sokolov <[hidden email]> writes:
> > > I propose to add PortalDrop at the 'if (completed)' branch of
> > > exec_execute_message.
> >
> > This violates our wire protocol specification, which
> > specifically says
> >
> >     If successfully created, a named portal object lasts till the end of
> >     the current transaction, unless explicitly destroyed. An unnamed
> >     portal is destroyed at the end of the transaction, or as soon as the
> >     next Bind statement specifying the unnamed portal as destination is
> >     issued. (Note that a simple Query message also destroys the unnamed
> >     portal.)
> >
> > I'm inclined to think that your complaint would be better handled
> > by having the client send a portal-close command, if it's not
> > going to do something else immediately.
>
> I thought about it as well. Then, if I understand correctly,
> PQsendQueryGuts and PQsendQueryInternal in pipeline mode should send
> "close portal" (CP) message after "execute" message, right?

I don't think they should do that.  The portal remains open, and the
libpq interface does that.  The portal gets closed at end of transaction
without the need for any client message.  I think if the client wanted
to close the portal ahead of time, it would need a new libpq entry point
to send the message to do that.

(I didn't add a Close Portal message to PQsendQueryInternal in pipeline
mode precisely because there is no such message in PQsendQueryGuts.
I think it would be wrong to unconditionally add a Close Portal message
to any of those places.)

--
Álvaro Herrera                            39°49'30"S 73°17'W


Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Yura Sokolov
In reply to this post by Álvaro Herrera
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Yura Sokolov
In reply to this post by Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Álvaro Herrera
In reply to this post by Yura Sokolov
On 2021-May-27, Yura Sokolov wrote:

> Alvaro Herrera писал 2021-05-26 23:59:

> > I don't think they should do that.  The portal remains open, and the
> > libpq interface does that.  The portal gets closed at end of transaction
> > without the need for any client message.  I think if the client wanted
> > to close the portal ahead of time, it would need a new libpq entry point
> > to send the message to do that.
>
> - PQsendQuery issues Query message, and exec_simple_query closes its
>   portal.
> - people doesn't expect PQsendQueryParams to be different from
>   PQsendQuery aside of parameter sending. The fact that the portal
>   remains open is a significant, unexpected and undesired difference.
> - PQsendQueryGuts is used in PQsendQueryParams and PQsendQueryPrepared.
>   It is always sends empty portal name and always "send me all rows"
>   limit (zero). Both usages are certainly to just perform query and
>   certainly no one expects portal remains open.

Thinking about it some more, Yura's argument about PQsendQuery does make
sense -- since what we're doing is replacing the use of a 'Q' message
just because we can't use it when in pipeline mode, then it is
reasonable to think that the replacement ought to have the same
behavior.  Upon receipt of a 'Q' message, the portal is closed
automatically, and ISTM that that behavior should be preserved.

That change would not solve the problem he complains about, because IIUC
his framework is using PQsendQueryPrepared, which I'm not proposing to
change.  It just removes the other discrepancy that was discussed in the
thread.

The attached patch does it.  Any opinions?

--
Álvaro Herrera       Valdivia, Chile
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere."                (Lamar Owen)


Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Álvaro Herrera
In reply to this post by Álvaro Herrera
On 2021-Jun-07, Alvaro Herrera wrote:

> The attached patch does it.  Any opinions?

Eh, really attached.

--
Álvaro Herrera                            39°49'30"S 73°17'W
"No es bueno caminar con un hombre muerto"

0001-Add-Portal-Close-to-pipelined-PQsendQuery.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Álvaro Herrera
On 2021-Jun-09, Tom Lane wrote:

> I wrote:
> > It turns out that the problem is specific to SELECT FOR UPDATE, and
> > it happens because nodeLockRows is not careful to shut down the
> > EvalPlanQual mechanism it uses before returning NULL at the end of
> > a scan.  If EPQ has been fired, it'll be holding a tuple slot
> > referencing whatever tuple it was last asked about.  The attached
> > trivial patch seems to take care of the issue nicely, while adding
> > little if any overhead.  (A repeat call to EvalPlanQualEnd doesn't
> > do much.)

Thanks for researching that -- good find.

> BTW, to be clear: I think Alvaro's change is also necessary.
> If libpq is going to silently do something different in pipeline
> mode than it does in normal mode, it should strive to minimize
> the semantic difference.  exec_simple_query includes a PortalDrop,
> so we'd best do the same in pipeline mode.  But this LockRows
> misbehavior would be visible in other operating modes anyway.

Agreed.  I'll get it pushed after the patch I'm currently looking at.

--
Álvaro Herrera                            39°49'30"S 73°17'W


Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Yura Sokolov
In reply to this post by Álvaro Herrera
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Add PortalDrop in exec_execute_message

Álvaro Herrera
In reply to this post by Tom Lane-2
On 2021-Jun-09, Tom Lane wrote:

> BTW, to be clear: I think Alvaro's change is also necessary.
> If libpq is going to silently do something different in pipeline
> mode than it does in normal mode, it should strive to minimize
> the semantic difference.  exec_simple_query includes a PortalDrop,
> so we'd best do the same in pipeline mode.

Pushed that patch, thanks.

--
Álvaro Herrera       Valdivia, Chile
"At least to kernel hackers, who really are human, despite occasional
rumors to the contrary" (LWN.net)