Re: DEALLOCATE IF EXISTS
On Tue, Oct 9, 2012 at 4:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?ISO-8859-1?Q?S=E9bastien_Lardi=E8re?= <slardiere@hi-media.com> writes:
Indeed, brackets was not correct, it's better now (I think), and correct
some comments.Still wrong ... at the very least you missed copyfuncs/equalfuncs.
In general, when adding a field to a struct, it's good practice to
grep for all uses of that struct.
I don't see Sébastien's message, but I made the same mistake in my patch.
Another one is attached with copyfuncs and equalfuncs. I did a grep for
DeallocateStmt and I don't believe I have missed anything else.
Also, I'm changing the subject so as not to hijack this thread any further.
Attachments:
deallocate_if_exists.2.patchapplication/octet-stream; name=deallocate_if_exists.2.patchDownload
*** a/doc/src/sgml/ref/deallocate.sgml
--- b/doc/src/sgml/ref/deallocate.sgml
***************
*** 26,32 **** PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
! DEALLOCATE [ PREPARE ] { <replaceable class="parameter">name</replaceable> | ALL }
</synopsis>
</refsynopsisdiv>
--- 26,33 ----
<refsynopsisdiv>
<synopsis>
! DEALLOCATE [ PREPARE ] [ IF EXISTS ] <replaceable class="parameter">name</replaceable>
! DEALLOCATE [ PREPARE ] ALL
</synopsis>
</refsynopsisdiv>
***************
*** 59,64 **** DEALLOCATE [ PREPARE ] { <replaceable class="parameter">name</replaceable> | ALL
--- 60,75 ----
</varlistentry>
<varlistentry>
+ <term><literal>IF EXISTS</literal></term>
+ <listitem>
+ <para>
+ Do not throw an error if the prepared statement does not exist.
+ A notice is issued in this case.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><replaceable class="parameter">name</replaceable></term>
<listitem>
<para>
*** a/src/backend/commands/prepare.c
--- b/src/backend/commands/prepare.c
***************
*** 556,562 **** void
DeallocateQuery(DeallocateStmt *stmt)
{
if (stmt->name)
! DropPreparedStatement(stmt->name, true);
else
DropAllPreparedStatements();
}
--- 556,562 ----
DeallocateQuery(DeallocateStmt *stmt)
{
if (stmt->name)
! DropPreparedStatement(stmt->name, !stmt->missing_ok);
else
DropAllPreparedStatements();
}
***************
*** 582,587 **** DropPreparedStatement(const char *stmt_name, bool showError)
--- 582,591 ----
/* Now we can remove the hash table entry */
hash_search(prepared_queries, entry->stmt_name, HASH_REMOVE, NULL);
}
+ else
+ ereport(NOTICE,
+ (errmsg("prepared statement \"%s\" does not exist, skipping",
+ stmt_name)));
}
/*
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 3675,3680 **** _copyDeallocateStmt(const DeallocateStmt *from)
--- 3675,3681 ----
DeallocateStmt *newnode = makeNode(DeallocateStmt);
COPY_STRING_FIELD(name);
+ COPY_SCALAR_FIELD(missing_ok);
return newnode;
}
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1961,1966 **** static bool
--- 1961,1967 ----
_equalDeallocateStmt(const DeallocateStmt *a, const DeallocateStmt *b)
{
COMPARE_STRING_FIELD(name);
+ COMPARE_SCALAR_FIELD(missing_ok);
return true;
}
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 8571,8582 **** DeallocateStmt: DEALLOCATE name
--- 8571,8598 ----
{
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = $2;
+ n->missing_ok = FALSE;
$$ = (Node *) n;
}
| DEALLOCATE PREPARE name
{
DeallocateStmt *n = makeNode(DeallocateStmt);
n->name = $3;
+ n->missing_ok = FALSE;
+ $$ = (Node *) n;
+ }
+ | DEALLOCATE IF_P EXISTS name
+ {
+ DeallocateStmt *n = makeNode(DeallocateStmt);
+ n->name = $4;
+ n->missing_ok = TRUE;
+ $$ = (Node *) n;
+ }
+ | DEALLOCATE PREPARE IF_P EXISTS name
+ {
+ DeallocateStmt *n = makeNode(DeallocateStmt);
+ n->name = $5;
+ n->missing_ok = TRUE;
$$ = (Node *) n;
}
| DEALLOCATE ALL
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2575,2580 **** typedef struct DeallocateStmt
--- 2575,2581 ----
NodeTag type;
char *name; /* The name of the plan to remove */
/* NULL means DEALLOCATE ALL */
+ bool missing_ok; /* skip error if table missing */
} DeallocateStmt;
/*
*** a/src/test/regress/expected/prepare.out
--- b/src/test/regress/expected/prepare.out
***************
*** 178,180 **** SELECT name, statement, parameter_types FROM pg_prepared_statements
--- 178,183 ----
------+-----------+-----------------
(0 rows)
+ -- test DEALLOCATE IF EXISTS;
+ DEALLOCATE IF EXISTS unprepared_statement;
+ NOTICE: prepared statement "unprepared_statement" does not exist, skipping
*** a/src/test/regress/sql/prepare.sql
--- b/src/test/regress/sql/prepare.sql
***************
*** 75,77 **** SELECT name, statement, parameter_types FROM pg_prepared_statements
--- 75,80 ----
DEALLOCATE ALL;
SELECT name, statement, parameter_types FROM pg_prepared_statements
ORDER BY name;
+
+ -- test DEALLOCATE IF EXISTS;
+ DEALLOCATE IF EXISTS unprepared_statement;
On Tue, Oct 9, 2012 at 4:44 PM, Vik Reykja <vikreykja@gmail.com> wrote:
On Tue, Oct 9, 2012 at 4:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
=?ISO-8859-1?Q?S=E9bastien_Lardi=E8re?= <slardiere@hi-media.com> writes:
Indeed, brackets was not correct, it's better now (I think), and correct
some comments.Still wrong ... at the very least you missed copyfuncs/equalfuncs.
In general, when adding a field to a struct, it's good practice to
grep for all uses of that struct.I don't see Sébastien's message, but I made the same mistake in my patch.
Another one is attached with copyfuncs and equalfuncs. I did a grep for
DeallocateStmt and I don't believe I have missed anything else.Also, I'm changing the subject so as not to hijack this thread any further.
I am taking no comments to mean no objections and have added this to the
next commitfest.
On Tue, 09 Oct 2012 16:44:07 +0200, Vik Reykja <vikreykja@gmail.com> wrote:
I don't see Sébastien's message, but I made the same mistake in my patch.
Another one is attached with copyfuncs and equalfuncs. I did a grep for
DeallocateStmt and I don't believe I have missed anything else.
The patch looks pretty straightforward to me, except for one thing:
previously there was no NOTICE if you sent a Close message to the server
and the prepared statement didn't exist. But I'm leaving it for the
committer to decide whether that's a problem or not, and marking this one
Ready for Committer.
Regards,
Marko Tiikkaja
On 09.10.2012 17:44, Vik Reykja wrote:
On Tue, Oct 9, 2012 at 4:09 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
=?ISO-8859-1?Q?S=E9bastien_Lardi=E8re?=<slardiere@hi-media.com> writes:
Indeed, brackets was not correct, it's better now (I think), and correct
some comments.Still wrong ... at the very least you missed copyfuncs/equalfuncs.
In general, when adding a field to a struct, it's good practice to
grep for all uses of that struct.I don't see Sébastien's message, but I made the same mistake in my patch.
Another one is attached with copyfuncs and equalfuncs. I did a grep for
DeallocateStmt and I don't believe I have missed anything else.Also, I'm changing the subject so as not to hijack this thread any further.
I fail to see the point of DEALLOCATE IF EXISTS. Do you have real use
case for this, or was this just a case of adding IF EXISTS to all
commands for the sake of completeness?
Usually the client knows what statements have been prepared, but perhaps
you want to make sure everything is deallocated in some error handling
case or similar. But in that case, you might as well just issue a
regular DEALLOCATE and ignore errors. Or even more likely, you'll want
to use DEALLOCATE ALL.
- Heikki
--
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 27, 2012 at 3:15 PM, Heikki Linnakangas <hlinnakangas@vmware.com
wrote:
I fail to see the point of DEALLOCATE IF EXISTS. Do you have real use case
for this, or was this just a case of adding IF EXISTS to all commands for
the sake of completeness?Usually the client knows what statements have been prepared, but perhaps
you want to make sure everything is deallocated in some error handling case
or similar. But in that case, you might as well just issue a regular
DEALLOCATE and ignore errors. Or even more likely, you'll want to use
DEALLOCATE ALL.
Hmm. The test case I had for it, which was very annoying in an "I want to
be lazy" sort of way, I am unable to reproduce now. So I guess this
becomes a "make it like the others" and the community can decide whether
that's desirable.
In my personal case, which again I can't reproduce because it's been a
while since I've done it, DEALLOCATE ALL would have worked. I was
basically preparing a query to work on it in the same conditions that it
would be executed in a function, and I was only working on one of these at
a time so ALL would have been fine.
On 30.11.2012 12:05, Vik Reykja wrote:
On Tue, Nov 27, 2012 at 3:15 PM, Heikki Linnakangas<hlinnakangas@vmware.com
wrote:
I fail to see the point of DEALLOCATE IF EXISTS. Do you have real use case
for this, or was this just a case of adding IF EXISTS to all commands for
the sake of completeness?Usually the client knows what statements have been prepared, but perhaps
you want to make sure everything is deallocated in some error handling case
or similar. But in that case, you might as well just issue a regular
DEALLOCATE and ignore errors. Or even more likely, you'll want to use
DEALLOCATE ALL.Hmm. The test case I had for it, which was very annoying in an "I want to
be lazy" sort of way, I am unable to reproduce now. So I guess this
becomes a "make it like the others" and the community can decide whether
that's desirable.In my personal case, which again I can't reproduce because it's been a
while since I've done it, DEALLOCATE ALL would have worked. I was
basically preparing a query to work on it in the same conditions that it
would be executed in a function, and I was only working on one of these at
a time so ALL would have been fine.
Ok. Being the lazy person that I am, I'm going to just mark this as
rejected then. There is no consensus that we should decorate every DDL
command with "IF EXISTS", and even if we did, it's not clear that it
should include DEALLOCATE. But thanks for the effort anyway!
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Dec 5, 2012 at 9:12 AM, Heikki Linnakangas
<hlinnakangas@vmware.com>wrote:
Ok. Being the lazy person that I am, I'm going to just mark this as
rejected then. There is no consensus that we should decorate every DDL
command with "IF EXISTS", and even if we did, it's not clear that it should
include DEALLOCATE. But thanks for the effort anyway!
Fair enough. :-)
Thanks for taking a look at it.