Re: [HACKERS] Frontend/backend protocol improvements proposal (request).

From: Dmitriy Igrishin <dmitigr(at)gmail(dot)com>
To: Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>
Cc: "pgsql-general(at)postgresql(dot)org" <pgsql-general(at)postgresql(dot)org>
Subject: Re: [HACKERS] Frontend/backend protocol improvements proposal (request).
Date: 2013-06-24 13:55:23
Message-ID: CAAfz9KPX6a--CGXzJyugK+3-k=hU94pSqHaPM6nvFyTMav=Dfg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-general pgsql-hackers

2013/6/24 Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>

> I'm moving this discussion to -general.
>
Okay, lets continue here.

>
> Dmitriy Igrishin wrote:
> >>> While developing a C++ client library for Postgres I felt lack of extra
> >>> information in command tags in the CommandComplete (B) message [...]
> >>> for the following commands:
>
> >> It seems like bad design to me to keep a list of prepared statements
> >> on the client side when it is already kept on the server side
> >> (accessible with the pg_prepared_statements view).
> >>
> >> What's wrong with the following:
> >> If the user wants to deallocate an individual prepared statement,
> >> just send "DEALLOCATE <statement name>" to the server. If the
> >> statement does not exist, the server will return an error.
> >> If the user wants to deallocate all statements, just send
> >> "DEALLOCATE ALL".
> >> Why do you need to track prepared statements on the client side?
> >
> >
> > Nothing wrong if the user wants to deal with scary and cumbersome code.
> > As library author, I want to help people make things simpler.
>
> I don't think that anything would change on the user end.
>
But I think so.

>
> > To understand me, please look at the pseudo C++ code below.
> >
> >
> > // A class designed to work with prepared statements
> > class Prepared_statement {
> >
> > public:
> > // Methods to generate a Bind message, like
> > Prepared_statement* bind(Position, Value);
> > // ... and more
> > // Methods to send Execute message, like
> > void execute();
> > void execute_async();
> > };
> >
> > class Connection {
> > public:
> > // many stuff ...
> > void close();
> >
> > Prepared_statement* prepare(Name, Query);
> > void prepare_async(Statement);
> >
> > // Make yet another instance of prepared statement.
> > Prepared_statement* prepared_statement(Name);
> >
> > // etc.
> > };
> >
> > The Connection class is a factory for Prepared_statement instances.
> > As you can see, the Connection::prepare() returns new instance of
> > *synchronously* prepared statement. Next, the user can bind values
> > and execute the statement, like this:
> >
> > void f(Connection* cn)
> > {
> > // Prepare unnamed statement and execute it.
> > cn->prepare("SELECT $1::text")->bind(0, "Albe")->execute();
> > // Ps: don't worry about absence of delete; We are using smart
> pointers :-)
> > }
> >
> > But there is a another possible case:
> >
> > void f(Connection* cn)
> > {
> > Prepared_statement* ps = cn->prepare("SELECT $1::text");
> > cn->close(); // THIS SHOULD invalidate all Prepared_statement
> instances ...
> > ps->bind(0, "Albe"); // ... to throw the exception here
> > }
>
> Attempting to send a bind message over a closed connection
> will result in a PostgreSQL error. All you have to do is wrap
> that into an exception of your liking.
>
Okay, thanks for the info.

>
> > Moreover, consider:
> >
> > void f(Connection* cn)
> > {
> > Prepared_statement* ps1 = cn->prepare("ps1", "SELECT $1::text");
> >
> > cn->deallocate("ps1"); // THIS invalidates ps1 object...
>
> Shouldn't that be
> cn->deallocate(ps1);
> without quotes?
>
No, because Connection::deallocate(const string&) considered by me as a
wrapper over
DEALLOCATE SQL command. (As any other SQL command wrapper declared as the
Connection class member.) But it can be overloaded though, but there are
Prepared_statement::deallocate(void) (without arguments) instead.

>
> > ps1->bind(0, "Albe"); // ... to throw the exception here
> >
> >
> > Prepared_statement* ps2 = cn->prepare("ps2", "SELECT $1::text");
> >
> > cn->perform("DEALLOCATE ps2"); // THIS SHOULD ALSO invalidate ps2
> object...
> > ps2->bind(0, "Albe"); // ... to throw the exception here
> >
> > }
>
> Again, sending a bind message for a deallocated prepared statement
> will cause a PostgreSQL error automatically.
>
Thats great, but there is a some problem -- the *another* statement with
the same
name (and moreover with same parameters!) can be prepared after
deallocating.
And this can result in bugs. So, the client-side allocated "pointer to the
remote
statement" must be invalidated immediatly after deallocating.

>
> > In the latter case when the user deallocates named prepared statement
> directly,
> > the implementation of Connection can invalidates the prepared statement
> (ps2) by
> > analyzing and parsing CommandComplete command tag to get it's name.
> >
> > And please note, that the user can send DEALLOCATE asynchronously. And
> there is
> > only two ways to get the prepared statement (or another session
> object's) name:
> > 1) Parse the SQL command which the user is attempts to send;
> > 2) Just get it from CommandComplete command tag.
> >
> > I beleive that the 1) is a 100% bad idea.
> >
> > PS: this C++11 library is not publicaly available yet, but I hope it
> will this year.
>
> I still think that it is a bad idea to track this on the client side.
>
> What's wrong with throwing an exception when you get a PostgreSQL error?
> If you want to distinguish between certain error conditions,
> you can use the SQLSTATE. For example, trying to execute a deallocated
> statement would cause SQLSTATE 26000.
>
See above why it make sense.

// Dmitriy.

In response to

Responses

Browse pgsql-general by date

  From Date Subject
Next Message Tom Lane 2013-06-24 14:08:24 Re: Re: [HACKERS] Frontend/backend protocol improvements proposal (request).
Previous Message Dan Kogan 2013-06-24 13:44:09 Re: Standby stopped working after PANIC: WAL contains references to invalid pages

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2013-06-24 13:57:24 Re: Support for REINDEX CONCURRENTLY
Previous Message Tom Lane 2013-06-24 13:55:22 Re: [Review] Re: minor patch submission: CREATE CAST ... AS EXPLICIT