Обсуждение: NOT EXIST for PREPARE

Hello hackers.

Do I understand correctly the only way know availability PREPARE it will
appeal to pg_prepared_statements?
I think this is not a good practice. In some cases, we may not be aware of
the PREPARE made (pgpool).
Moreover, it seems popular question in the Internet:
http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists

What do you think about adding NOT EXIST functionality to PREPARE?

Thanks.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hello hackers.

Do I understand correctly the only way know availability PREPARE it will appeal to pg_prepared_statements?
I think this is not a good practice. In some cases, we may not be aware of the PREPARE made (pgpool). Moreover, it seems popular question in the Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists

What do you think about adding NOT EXIST functionality to PREPARE?

Fabrízio de Royes Mello wrote:
> I think you meant IF NOT EXISTS, right?
Thanks, you right.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Fabrízio de Royes Mello wrote:

I think you meant IF NOT EXISTS, right?

Thanks, you right.

Hi,


On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
> Do I understand correctly the only way know availability PREPARE it will
> appeal to pg_prepared_statements?
> I think this is not a good practice. In some cases, we may not be aware of
> the PREPARE made (pgpool). Moreover, it seems popular question in the
> Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
> 
> What do you think about adding NOT EXIST functionality to PREPARE?

Not very much. If you're not in in control of the prepared statements, you
can't be sure it's not an entirely different statement. So NOT EXISTS
doesn't really buy you anything, you'd still need to compare the
statement somehow.

Andres

Andres Freund wrote:
> you'd still need to compare the
> statement somehow

You right, I think about that as syntax sugar. Maybe with some performance
increase but hardly. We can save on a round trip.


It may be necessary to add an index on the field "statement"?

Thanks.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Fabrízio de Royes Mello wrote:
> You already have a patch? If yes I'm glad to review it.

If the community is not against it, I'll do it quickly. Changing the syntax
is the risk. In addition, we have already missed 9.6.

Thanks.


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-03-22 16:51:26 +0300, Yury Zhuravlev wrote:
> Andres Freund wrote:
> >you'd still need to compare the
> >statement somehow
> 
> You right, I think about that as syntax sugar. Maybe with some performance
> increase but hardly. We can save on a round trip.

If anything what'd be useful would be DEALLOCATE IF EXISTS; that'd allow
you to use prepare safely this way.

> It may be necessary to add an index on the field "statement"?

It's not an actual table, it's a view over a function.

Yury Zhuravlev wrote:
> It may be necessary to add an index on the field "statement"?
Sorry. Said nonsense.

We need to understand what kind of behavior should be if the name is the
same but query_string not? Replace?

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Mar 22, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
> Hi,
> On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
>> Do I understand correctly the only way know availability PREPARE it will
>> appeal to pg_prepared_statements?
>> I think this is not a good practice. In some cases, we may not be aware of
>> the PREPARE made (pgpool). Moreover, it seems popular question in the
>> Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
>>
>> What do you think about adding NOT EXIST functionality to PREPARE?
>
> Not very much. If you're not in in control of the prepared statements, you
> can't be sure it's not an entirely different statement. So NOT EXISTS
> doesn't really buy you anything, you'd still need to compare the
> statement somehow.

Strongly disagree!  A typical use case of this feature would be in
connection pooler scenarios where you *are* in control of the
statement but it's a race to see who creates it first.  This feature
should be immediately be incorporated by the JDBC driver so that we'd
no longer have to disable server side prepared statements when using
pgbounder (for example).

merlin

* Merlin Moncure (mmoncure@gmail.com) wrote:
> On Tue, Mar 22, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
> > On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
> >> Do I understand correctly the only way know availability PREPARE it will
> >> appeal to pg_prepared_statements?
> >> I think this is not a good practice. In some cases, we may not be aware of
> >> the PREPARE made (pgpool). Moreover, it seems popular question in the
> >> Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
> >>
> >> What do you think about adding NOT EXIST functionality to PREPARE?
> >
> > Not very much. If you're not in in control of the prepared statements, you
> > can't be sure it's not an entirely different statement. So NOT EXISTS
> > doesn't really buy you anything, you'd still need to compare the
> > statement somehow.
>
> Strongly disagree!  A typical use case of this feature would be in
> connection pooler scenarios where you *are* in control of the
> statement but it's a race to see who creates it first.  This feature
> should be immediately be incorporated by the JDBC driver so that we'd
> no longer have to disable server side prepared statements when using
> pgbounder (for example).

Indeed, and we already said we're not going to verify that the object
that already exists in a 'IF NOT EXISTS' case matches exactly whatever
you're trying to create, see CREATE TABLE.

I agree that PREPARE IF NOT EXISTS would be nice to have, but only if we
can keep it fast somehow, which is the part that makes me wonder a bit.

Having PREPARE IF NOT EXISTS would imply that application authors would
be expected to run a set of PREPAREs at the start of each transaction
(if you want to support transaction pooling mode in, say, pgbouncer),
for each prepared statement they want to use in that transaction.  That
doesn't seem completely unreasonable, but it'd need to be fast.

Thanks!

Stephen

On 2016-03-22 09:37:15 -0500, Merlin Moncure wrote:
> On Tue, Mar 22, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
> > Hi,
> > On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
> >> Do I understand correctly the only way know availability PREPARE it will
> >> appeal to pg_prepared_statements?
> >> I think this is not a good practice. In some cases, we may not be aware of
> >> the PREPARE made (pgpool). Moreover, it seems popular question in the
> >> Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
> >>
> >> What do you think about adding NOT EXIST functionality to PREPARE?
> >
> > Not very much. If you're not in in control of the prepared statements, you
> > can't be sure it's not an entirely different statement. So NOT EXISTS
> > doesn't really buy you anything, you'd still need to compare the
> > statement somehow.
> 
> Strongly disagree!  A typical use case of this feature would be in
> connection pooler scenarios where you *are* in control of the
> statement but it's a race to see who creates it first.  This feature
> should be immediately be incorporated by the JDBC driver so that we'd
> no longer have to disable server side prepared statements when using
> pgbounder (for example).

Uh. JDBC precisely is a scenario where that's *NOT* applicable? You're
not in control of the precise prepared statement names it generates, so
you have no guarantee that one prepared statement identified by its name
means the same in another connection.

You can use something like PREPARE IF NOT EXISTS across a statement
level pooler if, and only if, the prepared statements have a name that's
fixed and unique for each statement.

* Andres Freund (andres@anarazel.de) wrote:
> On 2016-03-22 09:37:15 -0500, Merlin Moncure wrote:
> > On Tue, Mar 22, 2016 at 8:01 AM, Andres Freund <andres@anarazel.de> wrote:
> > > Hi,
> > > On 2016-03-22 12:41:43 +0300, Yury Zhuravlev wrote:
> > >> Do I understand correctly the only way know availability PREPARE it will
> > >> appeal to pg_prepared_statements?
> > >> I think this is not a good practice. In some cases, we may not be aware of
> > >> the PREPARE made (pgpool). Moreover, it seems popular question in the
> > >> Internet: http://stackoverflow.com/questions/1193020/php-postgresql-check-if-a-prepared-statement-already-exists
> > >>
> > >> What do you think about adding NOT EXIST functionality to PREPARE?
> > >
> > > Not very much. If you're not in in control of the prepared statements, you
> > > can't be sure it's not an entirely different statement. So NOT EXISTS
> > > doesn't really buy you anything, you'd still need to compare the
> > > statement somehow.
> >
> > Strongly disagree!  A typical use case of this feature would be in
> > connection pooler scenarios where you *are* in control of the
> > statement but it's a race to see who creates it first.  This feature
> > should be immediately be incorporated by the JDBC driver so that we'd
> > no longer have to disable server side prepared statements when using
> > pgbounder (for example).
>
> Uh. JDBC precisely is a scenario where that's *NOT* applicable? You're
> not in control of the precise prepared statement names it generates, so
> you have no guarantee that one prepared statement identified by its name
> means the same in another connection.

Clearly, you'd need to be able to control the prepared statement name to
use such a feature.

Given that we're talking about what sounds like a new feature in the
JDBC driver, I don't see why you wouldn't also make that a requirement
of the feature..?  Or have the JDBC driver calculate a unique ID for
each statement using a good hash, perhaps?

Note: I don't pretend to have any clue as to the internals of the JDBC
driver, but it hardly seems far-fetched to have this be supported in a
way that works.

Thanks!

Stephen

Fabrízio de Royes Mello wrote:
> Skip error if already exists when catched in
> src/backend/commands/prepare.c isn't enough?

I think that's enough. And expand PrepareStmt of course.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> You already have a patch? If yes I'm glad to review it.
>
Please. Patch in attachment.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Yury Zhuravlev wrote:
>> You already have a patch? If yes I'm glad to review it.
>>
> Please. Patch in attachment.
>

Fix bug, forgot change attr number in parser.
And, I forgot example:
PREPARE usrrptplan (int) IF NOT EXISTS AS
    SELECT * FROM pg_operator;
PREPARE

New patch in attachment.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Yury Zhuravlev wrote:
> Fabrízio de Royes Mello wrote:
> >You already have a patch? If yes I'm glad to review it.
> 
> If the community is not against it, I'll do it quickly. Changing the syntax
> is the risk. In addition, we have already missed 9.6.

Also we're in the middle of a commitfest, and it would be polite to
review the patches that have been listed in it for almost two months
now.  We shouldn't distract reviewing power towards new patches at this
point.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I was not sure about the syntax, so this was a prototype. Now, like all
completed yet.

>
> 1) I think this syntax is wrong... Instead the common should be:
>
> PREPARE [IF NOT EXISTS] ...
You right. Done.


> 2) All of CINE statements we emit a NOTICE skipping message, so you should
> emit a message like it:
Done.


> 3) There are no regression tests
Done.
> 4) There are no docs
Done.


Regards.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Fabrízio de Royes Mello wrote:
>
> You're correct. Yury please add your patch to the next commitfest.
Done. But I do not have restrictions as part of our PostgresPro distr. I
think this patch will be in production a month.

Big Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

<div dir="ltr"><div class="gmail_extra">On Tue, Mar 22, 2016 at 6:04 PM, Yury Zhuravlev <<a
href="mailto:u.zhuravlev@postgrespro.ru">u.zhuravlev@postgrespro.ru</a>>wrote:<br />><br />> I was not sure
aboutthe syntax, so this was a prototype. Now, like all completed yet.<br />><br />>><br />>> 1) I think
thissyntax is wrong... Instead the common should be:<br />>><br />>> PREPARE [IF NOT EXISTS] ...<br
/>><br/>> You right. Done.<br />><br />><br />>> 2) All of CINE statements we emit a NOTICE skipping
message,so you should<br />>> emit a message like it:<br />><br />> Done.<br />><br />><br />>>
3)There are no regression tests<br />><br />> Done.<br />>><br />>> 4) There are no docs<br />><br
/>>Done.<br />><br /><br /></div><div class="gmail_extra">I got an error when build this patch.<br /><br />$
./configure--prefix=/home/fabrizio/pgsql --enable-cassert --enable-coverage --enable-tap-tests --enable-depend<br /><br
/>...<br/><br /></div><div class="gmail_extra">$ make<br /><br />...<br />gcc -Wall -Wmissing-prototypes
-Wpointer-arith-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security
-fno-strict-aliasing-fwrapv -fexcess-precision=standard -fprofile-arcs -ftest-coverage  -pthread -D_REENTRANT
-D_THREAD_SAFE-D_POSIX_PTHREAD_SEMANTICS -DECPG_COMPILE -I../include -I../../../../src/interfaces/ecpg/include -I. -I.
-DMAJOR_VERSION=4-DMINOR_VERSION=12 -DPATCHLEVEL=0 -I../../../../src/include -D_GNU_SOURCE   -c -o preproc.o preproc.c
-MMD-MP -MF .deps/preproc.Po<br />preproc.y: In function ‘base_yyparse’:<br />preproc.y:8654:15: error: incompatible
typeswhen assigning to type ‘struct prep’ from type ‘char *’<br />  $$ = cat_str(5,mm_strdup("prepare if not
exists"),$5,$6,mm_strdup("as"),$8);<br/>               ^<br />make[4]: *** [preproc.o] Error 1<br />make[4]: Leaving
directory`/data/fabrizio/Dropbox/dev/postgresql/src/interfaces/ecpg/preproc'<br />make[3]: *** [all-preproc-recurse]
Error2<br />make[3]: Leaving directory `/data/fabrizio/Dropbox/dev/postgresql/src/interfaces/ecpg'<br />make[2]: ***
[all-ecpg-recurse]Error 2<br />make[2]: Leaving directory `/data/fabrizio/Dropbox/dev/postgresql/src/interfaces'<br
/>make[1]:*** [all-interfaces-recurse] Error 2<br />make[1]: Leaving directory
`/data/fabrizio/Dropbox/dev/postgresql/src'<br/>make: *** [all-src-recurse] Error 2<br /><br /><br /></div><div
class="gmail_extra"><br/></div><div class="gmail_extra">I also didin't see no psql tab-complete code.<br /></div><div
class="gmail_extra"><br/><br />Regards,<br /></div><div class="gmail_extra"><br />--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

Fabrízio de Royes Mello wrote:
> I got an error when build this patch.
Thanks for review and tests!

I'm working on it. Without ecpg everything works fine.
In ecpg we have black perl magic and special rules for PREPARE.
The error is in perl script.

Regards
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Mar 23, 2016 at 7:27 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> With PREPARE IF NOT EXISTS the client is also paying parse and network
> overhead for every time you send that statement. Much better not to send it
> repeatedly in the first place.

How did you determine that?  The client prepares the statement exactly
once.  The problem is there's no easy way to determine if someone else
prepared it first and this patch directly addresses that.

> I think we need to take a step back here and better define the problem
> before stepping in with a proposed solution. Something that avoids the need
> to spam the server with endless copies of the same PREPARE statements would
> be good.

I'm not understanding the objection at all.  You have N client
sessions connecting to the database that all utilize the same named
prepared statement.  A typical pattern is for the application to
prepare them all upon startup, but currently each PREPARE needs to be
wrapped with an exception handler in case someone else prepared it
first.  Having an IF NOT EXISTS decoration simplifies this.  This can
happen both inside and outside of connection pooling scenarios.

merlin

Fabrízio de Royes Mello wrote:
> I got an error when build this patch.

I fix it! All tests pass (include ecpg tests).
Patch in attachment.

Thanks.

PS Who use ecpg? For me it's like hell.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> PS Who use ecpg? For me it's like hell. 

More than you think.

I doubt you want to propose removing features that make developing new
features harder, or do you? :)

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Wed, Mar 23, 2016 at 8:21 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
> I'm not understanding the objection at all.  You have N client
> sessions connecting to the database that all utilize the same named
> prepared statement.  A typical pattern is for the application to
> prepare them all upon startup, but currently each PREPARE needs to be
> wrapped with an exception handler in case someone else prepared it
> first.  Having an IF NOT EXISTS decoration simplifies this.  This can
> happen both inside and outside of connection pooling scenarios.

I'm walking that back a bit -- this is only interesting in pooler
scenarios, especially pgbouncer where you have no way of knowing if
the statement is created or not.  Of course, you can always re-prepare
them following a discard but that's quite pessimal in many cases.
Still, I've often wanted this exact feature.

merlin

2016-03-23 16:21 GMT+03:00 Merlin Moncure <mmoncure@gmail.com>:
> On Wed, Mar 23, 2016 at 7:27 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
Craig>> With PREPARE IF NOT EXISTS the client is also paying parse and network
Craig>> overhead for every time you send that statement. Much better
not to send it
Craig>> repeatedly in the first place.
>
Merlin> How did you determine that?  The client prepares the statement exactly
Merlin> once.  The problem is there's no easy way to determine if someone else
Merlin> prepared it first and this patch directly addresses that.

With suggested "prepare if not exists", client would still have to send full
query text along with "prepare if not exist" command.
That is "network overhead".
DB would still have to check if the same query with same "query name" has
already been registered. Well, that query check would probably be easier than
"generate execution plan", yet it can be of non-zero overhead.

Craig>> I think we need to take a step back here and better define the problem
Craig>> before stepping in with a proposed solution. Something that
avoids the need
Craig>> to spam the server with endless copies of the same PREPARE
statements would
Craig>> be good.

+1

Merlin> A typical pattern is for the application to
Merlin> prepare them all upon startup, but currently each PREPARE needs to be
Merlin> wrapped with an exception handler in case someone else prepared it
Merlin> first.

If you plan to have "prepare if not exists" at startup only, why don't
you just wrap it with
exception handler then?

If you plan to always issue "prepare if not exists", then you will
have to send full query text
for each prepare => overhead. Those repeated query texts are
"endless copies of the same PREPARE statements" Craig is talking about.

Merlin>The client prepares the statement exactly
Merlin>once.  The problem is there's no easy way to determine if someone else
Merlin>prepared it first

Merlin, if by "client" you somehow mean JDBC (e.g. pgjdbc), then it
does track which connections
have which queries prepared.
So the thing we/you need might be not backend support for "prepare if
not exists", but some kind of
bouncer vs jdbc integration, so jdbc knows which connection it is
using at each point in time.

Vladimir

On Wed, Mar 23, 2016 at 12:46 PM, Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
> 2016-03-23 16:21 GMT+03:00 Merlin Moncure <mmoncure@gmail.com>:
> Merlin> A typical pattern is for the application to
> Merlin> prepare them all upon startup, but currently each PREPARE needs to be
> Merlin> wrapped with an exception handler in case someone else prepared it
> Merlin> first.
>
> If you plan to have "prepare if not exists" at startup only, why don't
> you just wrap it with
> exception handler then?

That's impolite to our users.  Virtually all other commands have been
decorated with IF [NOT] exists to avoid having to guard with exception
handler -- why not this one?  Also, if the handler is on the client
side, it tends to be racey.

> If you plan to always issue "prepare if not exists", then you will
> have to send full query text
> for each prepare => overhead. Those repeated query texts are
> "endless copies of the same PREPARE statements" Craig is talking about.

No one is arguing that that you should send it any every time (at
least -- I hope not).

> Merlin>The client prepares the statement exactly
> Merlin>once.  The problem is there's no easy way to determine if someone else
> Merlin>prepared it first
>
> Merlin, if by "client" you somehow mean JDBC (e.g. pgjdbc), then it
> does track which connections
> have which queries prepared.

Again, not in pooling scenarios.  The problems integrating server side
prepared statements with pgbouncer are well known.

merlin

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:
> 2016-03-23 16:21 GMT+03:00 Merlin Moncure <mmoncure@gmail.com>:
>> On Wed, Mar 23, 2016 at 7:27 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Craig>> With PREPARE IF NOT EXISTS the client is also paying parse and network
> Craig>> overhead for every time you send that statement. Much better
> Craig>> not to send it repeatedly in the first place.

> Merlin> How did you determine that?  The client prepares the statement exactly
> Merlin> once.  The problem is there's no easy way to determine if someone else
> Merlin> prepared it first and this patch directly addresses that.

> With suggested "prepare if not exists", client would still have to send full
> query text along with "prepare if not exist" command.
> That is "network overhead".

Yes.  Also, the query would certainly go through the lex and raw-parse
steps (scan.l and gram.y) on the database side before we could get as far
as realizing that it's a PREPARE IF NOT EXISTS and we should check for
pre-existence of the statement name.  Those steps are a nontrivial
fraction of the full parse-analysis overhead, especially for simpler
statements.

So I wouldn't take it as a given that this is actually going to be
very efficient.  Some measurements would be important to make the case
that this is worth having.
        regards, tom lane

Merlin>No one is arguing that that you should send it any every time (at
least -- I hope not).

Well, what is your suggestion exactly?

pgjdbc is NOT using "prepare ..." sql command.
I'm inclined to suppose, it will not use "prepare..." even after your fix.

Merlin>Again, not in pooling scenarios
Merlin>The problems integrating server side
Merlin>prepared statements with pgbouncer are well known.

I'm afraid, they are not.

Your words are "This feature should be immediately be incorporated
by the JDBC driver" yet you have not raised that subject on pgsql-jdbc
mailing list/github issue. That is not very fair.

Let me just name an alternative, so you can see what "a step back" could be:
What if pg-bouncer generated _fake_ ParameterStatus(backend_id=...)
messages to pgjdbc?
Then pgjdbc can learn true backend session id and it can use proper
set of prepared statements. Basically, pgjdbc could have prepared statement
cache per backend_id.
Well, it is not a 100% solution, however it is yet another approach to
"pgbouncer" problem, and it will support all the PostgreSQL versions.

It fits into current frontend-backend protocol as all clients are supposed
to handle ParameterStatus messages, etc.

Vladimir

On Wed, Mar 23, 2016 at 1:15 PM, Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
> Merlin>No one is arguing that that you should send it any every time (at
> least -- I hope not).
>
> Well, what is your suggestion exactly?
>
> pgjdbc is NOT using "prepare ..." sql command.
> I'm inclined to suppose, it will not use "prepare..." even after your fix.

Maybe so (note, the fix is not mine).  I guess better stated, the
proposed would allow use of server side prepared statements with JDBC.

> Merlin>Again, not in pooling scenarios
> Merlin>The problems integrating server side
> Merlin>prepared statements with pgbouncer are well known.
>
> I'm afraid, they are not.

yes, they are.   see:
https://pgbouncer.github.io/faq.html#how-to-use-prepared-statements-with-session-pooling

There are numerous pages on the web suggesting to DISCARD ALL in
transaction mode which is incredibly wasteful in the case of prepared
statements...so much so you're better off not using them if you can
help it.  With proposed, the application can simply prepare after
opening the 'connection' and not have to worry about handling the
error or scope.

> Your words are "This feature should be immediately be incorporated
> by the JDBC driver" yet you have not raised that subject on pgsql-jdbc
> mailing list/github issue. That is not very fair.

That's fair. Although there was a very long standing issue where jdbc
would try to prepare 'BEGIN' in such a a way that it could not be
disabled -- that was fixed.

merlin

Merlin>proposed would allow use of server side prepared statements with JDBC.

It would not. If we discuss end-to-end scenarios in detail, we would end up with
"send full query on each execution" -> lex/gram on each execution kind
of overheads.

That is hardly a proper way of using prepared statements.
I'm not eager to merge half-a-solution to pgjdbc.
Just in case: I'm one of those who maintain pgjdbc.

Merlin>https://pgbouncer.github.io/faq.html#how-to-use-prepared-statements-with-session-pooling
pgbouncer's docs> the reset query must clean old prepared statements

I'm afraid, that is just "you must clean prepared statements".
Where are the issues/suggestions from pgbouncer team?
I'm just a year into pgjdbc, I've attended a couple of the largest
PostgreSQL conferences
(like 700+ attendees), however I know of exactly 0 suggestions on
improving the matter.
Everybody just keeps saying "discard all".

Merlin>With proposed, the application can simply prepare after
Merlin>opening the 'connection' and not have to worry about handling the
Merlin>error or scope.

Can you name at least a couple of applications that indeed
"prepare after opening the connection"?
Note: it is not something JDBC driver can do. That "prepare after opening"
requires cooperation from the application.

I'm afraid, I know exactly 0 such applications. At least, in java world.
Applications typically use framework generated queries, so it would be
hard to impossible to "simply prepare after opening".
The "set of used sql queries" is likely to be infinite even for a
single application.
That is sad, but true.

That is exactly the reason why I've implemented _transparent_ statement cache
for pgjdbc. As application is using generated queries, pgjdbc detects
query reuse and enables server-prepared queries behind the scenes.


If no one objects, I would just go ahead and file
"report ParameterStatus(pgbouncer.backend_id=...)" issue for pgbouncer.
I guess we can agree on the name and semantics of the new status message,
so it would not accidentally collide with the one of newer PG version.

Merlin>Although there was a very long standing issue where jdbc
Merlin>would try to prepare 'BEGIN' in such a a way that it could not be
Merlin>disabled -- that was fixed.

What bothers me is current pgjdbc CI has exactly 0 pgbouncer tests.
That means "BEGIN is fixed" can easily break and no one would notice that.
It is tracked under https://github.com/pgjdbc/pgjdbc/issues/509, so
if there's interest in pgbouncer vs pgjdbc, then #509 might be a good start.

Vladimir

On Wed, Mar 23, 2016 at 2:17 PM, Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
> Merlin>proposed would allow use of server side prepared statements with JDBC.
>
> It would not. If we discuss end-to-end scenarios in detail, we would end up with
> "send full query on each execution" -> lex/gram on each execution kind
> of overheads.

I think we're talking over each other here.  I'm not suggesting the
jdbc driver needs to be adjusted.  All I'm saying is that the use of
server side prepared statements is extremely problematic in
conjunction with pgbouncer (or any technology where the application db
connection and the server session are not 1:1) and trivial with the
proposed patch.

Any discussion regarding jdbc is off topic relative to that.

merlin

Merlin,

* Merlin Moncure (mmoncure@gmail.com) wrote:
> No one is arguing that that you should send it any every time (at
> least -- I hope not).

I'm not sure I follow how you can avoid that though?

pgbouncer in transaction pooling mode may let a particular connection
die off and then, when you issue a new request, create a new one- which
won't have any prepared queries in it, even though you never lost your
connection to pgbouncer.

That's why I was saying you'd have to send it at the start of every
transaction, which does add to network load and requires parsing, etc.
Would be nice to avoid that, if possible, but I'm not quite sure how.

One thought might be to have the server somehow have a pre-canned set of
queries already set up and ready for you to use when you connect,
without any need to explicitly prepare them, etc.

Just a thought.  I do still like the general idea of INE support for
PREPARE, but perhaps there's a better option.

Thanks!

Stephen

Merlin> All I'm saying is that the use of
Merlin> server side prepared statements is extremely problematic in
Merlin> conjunction with pgbouncer

I've filed https://github.com/pgbouncer/pgbouncer/issues/126 to get
pgbouncer improved in regard to prepared statements.

Vladimir

Michael Meskes wrote:
> More than you think.
>
> I doubt you want to propose removing features that make developing new
> features harder, or do you? :)

I want to understand the situation. You may want to make the build ecpg
optional. Personally, I want to.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

> I want to understand the situation. You may want to make the build
> ecpg 
> optional. Personally, I want to.

You lost me here, sorry. What exactly do you want to do? 

While ecpg may not be the choice for new applications, there are a lot
of legacy applications out there that need ecpg to be migrated to
PostgreSQL. So I think, completely removing it is out of the question.

An optional build does not change a thing because it still has to
compile et al. If you mean you'd like to decouple it from the backend
build, that one is difficult because the parser is supposed to accept
exactly the same SQL. And we spend quite a bit of effort to make it
auto-build from the backend parser to make sure we don't lose any
changes. 

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL

Michael Meskes wrote:
> While ecpg may not be the choice for new applications, there are a lot
> of legacy applications out there that need ecpg to be migrated to
> PostgreSQL.

2016 is a good time to rewrite them. ;)
I think Postgres will be more likely if it would be a little less concerned
about compatibility IMHO.
But I will not insist. My problem with ecpg I decided.

Thanks.
--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

I have a big question. What need to do with message protocol?
If we write name in Parse message we store prepared statement.
I see some solutions for this problem but all not ideal:
1. We can add second char token for parse message. But too serious change.
2. We can try add parameter to tail of message. But in tail we have
variable length array with parameters.
3. Detect prefix of prepared name. For example "__". Effects think clear.

I would like to know the community opinion on this matter.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Craig>I really, really doubt you can change this before we do a
protocol version bump.

Technically speaking, the idea of using first bytes of statement name
to convey extra information does not require protocol version bump. It
can be backward and forward compatible.

For instance: if statement name starts with __, then skip throwing an
error if statement with exactly same text and parameter types has
already been prepared.

by "prepare ..." below I mean v3 prepare message, not "prepare sql" command.

prepare __s1(int) select $1; -> prepared
prepare __s1(int) select $1; -> prepared, no error since statement
name starts with __
prepare s1(int) select $1; -> prepared
prepare s1(int) select $1; -> error "statement s1 has already been used"

>doesn't have any kind of capabilities negotiation

Do you think capability negotiation should indeed be at the protocol level?
What's wrong with say "select * from backend_capabilities" at the
connection setup?

Vladimir

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:
> Craig>I really, really doubt you can change this before we do a
> protocol version bump.

> Technically speaking, the idea of using first bytes of statement name
> to convey extra information does not require protocol version bump. It
> can be backward and forward compatible.

> For instance: if statement name starts with __, then skip throwing an
> error if statement with exactly same text and parameter types has
> already been prepared.

If you think that's not a protocol change, you are mistaken.  It
changes a behavior that's specified in the protocol documentation.
        regards, tom lane

Tom>If you think that's not a protocol change, you are mistaken.  It
Tom>changes a behavior that's specified in the protocol documentation.

Even if it requires documentation, this particular change will work seamlessly
across existing implementations of v3 protocol.

For instance, it would not require to update pgbouncer to support that
__ convention.
In other words, __ convention is transparent to pgbouncer.

Consider Prepare2 kind of message is added. Then it would require to update
virtually every software that talks v3 protocol.

That is why I say that "some kind of __ convention" does not require protocol
version bump, while "adding new message" does require the bump.

Just to be clear: I'm not fond of encoding the answer to the universe
into statement name.
However, I find that "name convention" a smart invention.

Vladimir

Vladimir Sitnikov wrote:
> Just to be clear: I'm not fond of encoding the answer to the universe
> into statement name.
> However, I find that "name convention" a smart invention.

I forgot one more decision: add GUC variable.
A little fatty for this but not touch the protocol and easy to implement.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Vladimir Sitnikov <sitnikov.vladimir@gmail.com> writes:
> Tom>If you think that's not a protocol change, you are mistaken.  It
> Tom>changes a behavior that's specified in the protocol documentation.

> Even if it requires documentation, this particular change will work seamlessly
> across existing implementations of v3 protocol.

No, because it would break applications that are not expecting prepared
statement names starting with '__' to work differently than they did
before.  Not to mention that the whole idea of that being a semantically
significant property of a name is a monstrous kluge.
        regards, tom lane

Tom> Not to mention that the whole idea of that being a semantically
Tom> significant property of a name is a monstrous kluge.

You are right here.

Just in case, Marko Kreen says (see [1]) pgbouncer has all the information
required to remap statement names, so he says pgbouncer needs no
cooperation from backend nor from app side in order to implement
prepared statements properly.

[1]: https://github.com/pgbouncer/pgbouncer/issues/126#issuecomment-200900171

Vladimir

Tom Lane wrote:
> because it would break applications
I still do not agree with this. The app expects that there can be no
mistakes and it does not happen.
I can not invent a situation when it is breaks.

Thanks.

--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 03/23/2016 09:10 PM, Stephen Frost wrote:
> * Merlin Moncure (mmoncure@gmail.com) wrote:
>> No one is arguing that that you should send it any every time (at
>> least -- I hope not).
>
> I'm not sure I follow how you can avoid that though?
>
> pgbouncer in transaction pooling mode may let a particular connection
> die off and then, when you issue a new request, create a new one- which
> won't have any prepared queries in it, even though you never lost your
> connection to pgbouncer.
>
> That's why I was saying you'd have to send it at the start of every
> transaction, which does add to network load and requires parsing, etc.
> Would be nice to avoid that, if possible, but I'm not quite sure how.
>
> One thought might be to have the server somehow have a pre-canned set of
> queries already set up and ready for you to use when you connect,
> without any need to explicitly prepare them, etc.

Personally I think the right solution would be to add support for 
prepared statements in pgbouncer, and have pgbouncer run PREPARE as 
necessary, either after opening a new connection to the database or at 
the first use of a given prepared statement in a connection.

Application level connection poolers with prepared statement support, 
e.g. sequel for Ruby, does not need any special support from PostgreSQL 
and work just fine with our current feature set.

Andreas

On Thu, Mar 24, 2016 at 2:52 PM, Andreas Karlsson <andreas@proxel.se> wrote:
> On 03/23/2016 09:10 PM, Stephen Frost wrote:
>>
>> * Merlin Moncure (mmoncure@gmail.com) wrote:
>>>
>>> No one is arguing that that you should send it any every time (at
>>> least -- I hope not).
>>
>>
>> I'm not sure I follow how you can avoid that though?
>>
>> pgbouncer in transaction pooling mode may let a particular connection
>> die off and then, when you issue a new request, create a new one- which
>> won't have any prepared queries in it, even though you never lost your
>> connection to pgbouncer.
>>
>> That's why I was saying you'd have to send it at the start of every
>> transaction, which does add to network load and requires parsing, etc.
>> Would be nice to avoid that, if possible, but I'm not quite sure how.
>>
>> One thought might be to have the server somehow have a pre-canned set of
>> queries already set up and ready for you to use when you connect,
>> without any need to explicitly prepare them, etc.
>
> Personally I think the right solution would be to add support for prepared
> statements in pgbouncer, and have pgbouncer run PREPARE as necessary, either
> after opening a new connection to the database or at the first use of a
> given prepared statement in a connection.

maybe so. A while back I was running a hacked pgbouncer that had
support for async notifications (i still have the code around
somewhere).  It can be done -- however this not a complete solution;
both LISTEN and PREPARE are possible from within server-side
functions.

melrin

On 03/24/2016 11:21 PM, Merlin Moncure wrote:
>> Personally I think the right solution would be to add support for prepared
>> statements in pgbouncer, and have pgbouncer run PREPARE as necessary, either
>> after opening a new connection to the database or at the first use of a
>> given prepared statement in a connection.
>
> maybe so. A while back I was running a hacked pgbouncer that had
> support for async notifications (i still have the code around
> somewhere).  It can be done -- however this not a complete solution;
> both LISTEN and PREPARE are possible from within server-side
> functions.

Yes, it is not a complete solution, but it solves the problems of many 
users. I think even just supporting the protocol level prepare and 
execute commands would be enough for many of those who have problems 
with pgbouncer.

Andreas

On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Merlin,
>
> * Merlin Moncure (mmoncure@gmail.com) wrote:
>> No one is arguing that that you should send it any every time (at
>> least -- I hope not).
>
> I'm not sure I follow how you can avoid that though?
>
> pgbouncer in transaction pooling mode may let a particular connection
> die off and then, when you issue a new request, create a new one- which
> won't have any prepared queries in it, even though you never lost your
> connection to pgbouncer.
>
> That's why I was saying you'd have to send it at the start of every
> transaction, which does add to network load and requires parsing, etc.
> Would be nice to avoid that, if possible, but I'm not quite sure how.
>
> One thought might be to have the server somehow have a pre-canned set of
> queries already set up and ready for you to use when you connect,
> without any need to explicitly prepare them, etc.
>
> Just a thought.  I do still like the general idea of INE support for
> PREPARE, but perhaps there's a better option.

Admittedly, you make some pretty good points here.  I guess one
strategy would be to move them all to a function that sets an advisory
lock to signal they've been prepared.  That's pretty safe even in
multi-threaded scenarios since only one thread can send queries to the
backend at a time.  Advisory locks are pretty arcane though.  Still,
I'm coming round to your (and Andres's) point of view. :/

merlin



merlin

On Fri, Mar 25, 2016 at 8:36 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> Just a thought.  I do still like the general idea of INE support for
>> PREPARE, but perhaps there's a better option.
>
> Admittedly, you make some pretty good points here.  I guess one
> strategy would be to move them all to a function that sets an advisory
> lock to signal they've been prepared.  That's pretty safe even in
> multi-threaded scenarios since only one thread can send queries to the
> backend at a time.  Advisory locks are pretty arcane though.  Still,
> I'm coming round to your (and Andres's) point of view. :/

I signed up to review this patch as I thought I'd be a pretty fair
arbitrator of the "is this or is this not actually useful?" debate.  I
was initially pretty enthusiastic about this patch but after reviewing
the various objections I've gradually come around to the opinion that
the author really ought to demonstrate specific use cases where the
new syntax actually helps with pain points.  On the one hand, IF NOT
EXISTS is seems pretty harmless but on the other we try not to accept
patches for SQL level features that will not (or should not) ever by
used.

So, to the OP, if you could reiterate some specific cases where you
think this would be useful, that will help move the review forward.
(if not, it makes it more likely you'll get marked, "returned with
feedback" -- FYI, and thanks.  Note, I'm not the final word here but I
think I speak for the prevailing opinion in this regard.

merlin

On Fri, May 6, 2016 at 2:38 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 8:36 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
>> On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> Just a thought.  I do still like the general idea of INE support for
>>> PREPARE, but perhaps there's a better option.
>>
>> Admittedly, you make some pretty good points here.  I guess one
>> strategy would be to move them all to a function that sets an advisory
>> lock to signal they've been prepared.  That's pretty safe even in
>> multi-threaded scenarios since only one thread can send queries to the
>> backend at a time.  Advisory locks are pretty arcane though.  Still,
>> I'm coming round to your (and Andres's) point of view. :/
>
> I signed up to review this patch as I thought I'd be a pretty fair
> arbitrator of the "is this or is this not actually useful?" debate.  I
> was initially pretty enthusiastic about this patch but after reviewing
> the various objections I've gradually come around to the opinion that
> the author really ought to demonstrate specific use cases where the
> new syntax actually helps with pain points.  On the one hand, IF NOT
> EXISTS is seems pretty harmless but on the other we try not to accept
> patches for SQL level features that will not (or should not) ever by
> used.

Yeah.  I would assume that if you have a large number of statements
that you want to potentially prepare, it would be smarter to first
issue "SELECT name FROM pg_prepared_statements", and then prepare any
you need that aren't already there.  Blindly pre-preparing everything
doesn't seem like it will be good for performance, and if you do want
to do it for some reason, you can always ignore the error on the
client side.  So I'm not really seeing the use case for this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 6, 2016 at 2:38 PM, Merlin Moncure <mmoncure@gmail.com> wrote:
> On Fri, Mar 25, 2016 at 8:36 AM, Merlin Moncure <mmoncure@gmail.com> wrote:
>> On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> Just a thought.  I do still like the general idea of INE support for
>>> PREPARE, but perhaps there's a better option.
>>
>> Admittedly, you make some pretty good points here.  I guess one
>> strategy would be to move them all to a function that sets an advisory
>> lock to signal they've been prepared.  That's pretty safe even in
>> multi-threaded scenarios since only one thread can send queries to the
>> backend at a time.  Advisory locks are pretty arcane though.  Still,
>> I'm coming round to your (and Andres's) point of view. :/
>
> I signed up to review this patch as I thought I'd be a pretty fair
> arbitrator of the "is this or is this not actually useful?" debate.  I
> was initially pretty enthusiastic about this patch but after reviewing
> the various objections I've gradually come around to the opinion that
> the author really ought to demonstrate specific use cases where the
> new syntax actually helps with pain points.  On the one hand, IF NOT
> EXISTS is seems pretty harmless but on the other we try not to accept
> patches for SQL level features that will not (or should not) ever by
> used.

Yeah.  I would assume that if you have a large number of statements
that you want to potentially prepare, it would be smarter to first
issue "SELECT name FROM pg_prepared_statements", and then prepare any
you need that aren't already there.  Blindly pre-preparing everything
doesn't seem like it will be good for performance, and if you do want
to do it for some reason, you can always ignore the error on the
client side.  So I'm not really seeing the use case for this.

On Sun, Jun 5, 2016 at 11:33 AM, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> As an aside; most (all?) of our INEs apply to persistent schema objects.
> Extending that to session objects is a conceptual leap.

There is close to no activity here, so I marked the patch as returned
with feedback. I am doubtful about the performance btw..
-- 
Michael