Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Pavel Stehule escribió: I though about it too. But I didn't continue - reasons was named by Dean - and RemoveObjects are not difficult code - lot of code is mechanical - and it is not on critical path. I have pushed it after some editorialization. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello 2014/1/22 Dean Rasheed dean.a.rash...@gmail.com On 21 January 2014 22:28, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I have been mulling over this patch, and I can't seem to come to terms with it. I first started making it look nicer here and there, thinking it was all mostly okay, but eventually arrived at the idea that it seems wrong to do what this does: basically, get_object_address() tries to obtain an object address, and if that fails, return InvalidOid; then, in RemoveObjects, we try rather hard to figure out why that failed, and construct an error message. It seems to me that it would make more sense to have get_object_address somehow return a code indicating what failed; then we don't have to go all over through the parser code once more. Perhaps, for example, when missing_ok is given as true to get_object_address it also needs to get a pointer to ObjectType and a string; if some object does not exist then fill the ObjectType with the failing object and the string with the failing name. Then RemoveObjects can construct a string more easily. Not sure how workable this exact idea is; maybe there is a better way. Yeah, when I initially started reviewing this patch I had a very similar thought. But when I looked more deeply at the code beneath get_object_address, I started to doubt whether it could be done without rather extensive changes all over the place. Also get_object_address is itself called from a lot of places (not necessarily all in our code) and all the other places (in our code, at least) pass missing_ok=false. So it seemed rather ugly to change its signature and force a matching change in all those other places, which actually don't care about missing objects. Perhaps the answer would be to have a separate get_object_address_if_exists function, and remove the missing_ok flag from get_object_address, but that all felt like a much larger patch. In the end, I felt that Pavel's approach wasn't adding that much new code, and it's all localised in the one place that does actually tolerate missing objects. I though about it too. But I didn't continue - reasons was named by Dean - and RemoveObjects are not difficult code - lot of code is mechanical - and it is not on critical path. Regards Pavel I admit though, that I didn't explore the other approach very deeply, so perhaps it might fall out more neatly than I feared. Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
I have been mulling over this patch, and I can't seem to come to terms with it. I first started making it look nicer here and there, thinking it was all mostly okay, but eventually arrived at the idea that it seems wrong to do what this does: basically, get_object_address() tries to obtain an object address, and if that fails, return InvalidOid; then, in RemoveObjects, we try rather hard to figure out why that failed, and construct an error message. It seems to me that it would make more sense to have get_object_address somehow return a code indicating what failed; then we don't have to go all over through the parser code once more. Perhaps, for example, when missing_ok is given as true to get_object_address it also needs to get a pointer to ObjectType and a string; if some object does not exist then fill the ObjectType with the failing object and the string with the failing name. Then RemoveObjects can construct a string more easily. Not sure how workable this exact idea is; maybe there is a better way. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 21 January 2014 22:28, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I have been mulling over this patch, and I can't seem to come to terms with it. I first started making it look nicer here and there, thinking it was all mostly okay, but eventually arrived at the idea that it seems wrong to do what this does: basically, get_object_address() tries to obtain an object address, and if that fails, return InvalidOid; then, in RemoveObjects, we try rather hard to figure out why that failed, and construct an error message. It seems to me that it would make more sense to have get_object_address somehow return a code indicating what failed; then we don't have to go all over through the parser code once more. Perhaps, for example, when missing_ok is given as true to get_object_address it also needs to get a pointer to ObjectType and a string; if some object does not exist then fill the ObjectType with the failing object and the string with the failing name. Then RemoveObjects can construct a string more easily. Not sure how workable this exact idea is; maybe there is a better way. Yeah, when I initially started reviewing this patch I had a very similar thought. But when I looked more deeply at the code beneath get_object_address, I started to doubt whether it could be done without rather extensive changes all over the place. Also get_object_address is itself called from a lot of places (not necessarily all in our code) and all the other places (in our code, at least) pass missing_ok=false. So it seemed rather ugly to change its signature and force a matching change in all those other places, which actually don't care about missing objects. Perhaps the answer would be to have a separate get_object_address_if_exists function, and remove the missing_ok flag from get_object_address, but that all felt like a much larger patch. In the end, I felt that Pavel's approach wasn't adding that much new code, and it's all localised in the one place that does actually tolerate missing objects. I admit though, that I didn't explore the other approach very deeply, so perhaps it might fall out more neatly than I feared. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/12/8 Dean Rasheed dean.a.rash...@gmail.com On 7 December 2013 21:34, Pavel Stehule pavel.steh...@gmail.com wrote: Well I was basically proposing that does_not_exist_skipping() be enhanced to report on non-existent types that form part of the object specification. I think this would affect the CAST, FUNCTION, AGGREGATE and OPERATOR cases, but should be a fairly trivial extension to the code that you've already added. ok, updated patch is in attachment Cool. This looks good to me, except I found a corner case --- the type name for an operator may be NONE, in which case the typeName in the list will be NULL, so that needs to be guarded against. Updated patch attached. I think this is a good patch. It makes all the DROP...IF EXISTS commands consistently fault-tolerant, instead of the current 50/50 mix, and all the resulting NOTICEs give useful information about why objects don't exist and are being skipped. I think this is now ready for committer. thank you :) Pavel Nice work! Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello 2013/12/5 Peter Eisentraut pete...@gmx.net Can someone in this thread clarify the commit fest situation? I see two entries that appear to be the same: https://commitfest.postgresql.org/action/patch_view?id=1174 https://commitfest.postgresql.org/action/patch_view?id=1175 I think the first one is a duplicate or obsolete. no, both are valid, and every solve different issue. https://commitfest.postgresql.org/action/patch_view?id=1175 it implements fully fault tolerant DROP IF EXISTS statements. This patch is prerequisite for second patch https://commitfest.postgresql.org/action/patch_view?id=1174 This is a implementation of new pg_dump option --if-exists. This option ensure using fault tolerant DROPs statement by dump. Regards Pavel
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/12/5 Dean Rasheed dean.a.rash...@gmail.com On 5 December 2013 01:33, Peter Eisentraut pete...@gmx.net wrote: Can someone in this thread clarify the commit fest situation? I see two entries that appear to be the same: https://commitfest.postgresql.org/action/patch_view?id=1174 https://commitfest.postgresql.org/action/patch_view?id=1175 I think the first one is a duplicate or obsolete. #1174 looks to be a separate feature. I don't think it's dependent on #1175 from a code standpoint, but it probably needs it to work properly in all situations. I think #1175 is close to being ready for commit. Pavel, will you produce an updated patch based on our last discussion? I'll set this patch to waiting on author. I expected so your version was a final. I have no problem to do other enhancing (by me) , but I don't fully understand to your last proposal. Can you specify it more, please? Regards Pavel Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 5 December 2013 10:06, Pavel Stehule pavel.steh...@gmail.com wrote: I think #1175 is close to being ready for commit. Pavel, will you produce an updated patch based on our last discussion? I'll set this patch to waiting on author. I expected so your version was a final. I have no problem to do other enhancing (by me) , but I don't fully understand to your last proposal. Can you specify it more, please? Well I was basically proposing that does_not_exist_skipping() be enhanced to report on non-existent types that form part of the object specification. I think this would affect the CAST, FUNCTION, AGGREGATE and OPERATOR cases, but should be a fairly trivial extension to the code that you've already added. It should be possible to make the notice text from DROP...IF EXISTS consistent with the error text from a plain DROP. For example consider cases like func_name(no_such_schema.typename) and func_name(existing_schema.no_such_type). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2 December 2013 04:55, Pavel Stehule pavel.steh...@gmail.com wrote: Hello it looks well, thank you Regards Pavel I've been thinking about this some more, and there's another case that concerns me slightly. We're now making some of the DROP...IF EXISTS commands tolerate non-existent types as well as non-existent schemas --- functions, aggregates, casts and operators all have type names in their specifications. Of course it's possible that the type is missing because it was in a schema that was dropped, so this change seems to be in spirit of what was discussed, but it seems like a change that might catch some people out. I think that, on balance, it is a sensible change, since if the type doesn't exist, the dependent object can't exist either, so DROP...IF EXISTS shouldn't be raising an error. However, I wonder if we should be issuing a more specific NOTICE in this case too --- i.e., check for non-existent types in the same way as we check for non-existent parent objects --- type_does_not_exist_skipping() and type_list_does_not_exist_skipping(). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/12/4 Dean Rasheed dean.a.rash...@gmail.com On 2 December 2013 04:55, Pavel Stehule pavel.steh...@gmail.com wrote: Hello it looks well, thank you Regards Pavel I've been thinking about this some more, and there's another case that concerns me slightly. We're now making some of the DROP...IF EXISTS commands tolerate non-existent types as well as non-existent schemas --- functions, aggregates, casts and operators all have type names in their specifications. Of course it's possible that the type is missing because it was in a schema that was dropped, so this change seems to be in spirit of what was discussed, but it seems like a change that might catch some people out. I think that, on balance, it is a sensible change, since if the type doesn't exist, the dependent object can't exist either, so DROP...IF EXISTS shouldn't be raising an error. However, I wonder if we should be issuing a more specific NOTICE in this case too --- i.e., check for non-existent types in the same way as we check for non-existent parent objects --- type_does_not_exist_skipping() and type_list_does_not_exist_skipping(). +1 Pavel Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Can someone in this thread clarify the commit fest situation? I see two entries that appear to be the same: https://commitfest.postgresql.org/action/patch_view?id=1174 https://commitfest.postgresql.org/action/patch_view?id=1175 I think the first one is a duplicate or obsolete. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 5 December 2013 01:33, Peter Eisentraut pete...@gmx.net wrote: Can someone in this thread clarify the commit fest situation? I see two entries that appear to be the same: https://commitfest.postgresql.org/action/patch_view?id=1174 https://commitfest.postgresql.org/action/patch_view?id=1175 I think the first one is a duplicate or obsolete. #1174 looks to be a separate feature. I don't think it's dependent on #1175 from a code standpoint, but it probably needs it to work properly in all situations. I think #1175 is close to being ready for commit. Pavel, will you produce an updated patch based on our last discussion? I'll set this patch to waiting on author. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Dean Rasheed escribió:
+/*
+ * If a schema was explicitly specified, test if it exists. If it does not,
+ * report the schema as missing rather than the child object.
+ */
+static bool
+schema_does_not_exist_skipping(List *objname,
+const char **msg,
+char **name)
+{
+ RangeVar*rel;
+
+ rel = makeRangeVarFromNameList(objname);
+
+ if (rel-schemaname != NULL
+ !OidIsValid(LookupNamespaceNoError(rel-schemaname)))
+ {
+ *msg = gettext_noop(schema \%s\ does not exist, skipping);
+ *name = rel-schemaname;
+
+ return true;
+ }
+
+ return false;
+}
In success cases, are we leaking a lot of memory? In the error case I
guess it doesn't matter that the RangeVar is getting leaked (we're
aborting anyway), but if we're called and everything turns out to work,
are things cleaned up timely?
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2 December 2013 19:37, Alvaro Herrera alvhe...@2ndquadrant.com wrote:
Dean Rasheed escribió:
+/*
+ * If a schema was explicitly specified, test if it exists. If it does not,
+ * report the schema as missing rather than the child object.
+ */
+static bool
+schema_does_not_exist_skipping(List *objname,
+const char **msg,
+char **name)
+{
+ RangeVar*rel;
+
+ rel = makeRangeVarFromNameList(objname);
+
+ if (rel-schemaname != NULL
+ !OidIsValid(LookupNamespaceNoError(rel-schemaname)))
+ {
+ *msg = gettext_noop(schema \%s\ does not exist, skipping);
+ *name = rel-schemaname;
+
+ return true;
+ }
+
+ return false;
+}
In success cases, are we leaking a lot of memory? In the error case I
guess it doesn't matter that the RangeVar is getting leaked (we're
aborting anyway), but if we're called and everything turns out to work,
are things cleaned up timely?
I think that memory gets freed at the end of the DROP command, so I
don't think this is a concern. In any case, that RangeVar is only of
order 50 bytes. If we were concerned about memory leakage here, a
bigger concern would be the calling code in does_not_exist_skipping(),
which is using NameListToString() which allocates at least 1024 bytes
for the name of the non-existent object without freeing it.
Regards,
Dean
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Dean Rasheed escribió: I think that memory gets freed at the end of the DROP command, so I don't think this is a concern. In any case, that RangeVar is only of order 50 bytes. If we were concerned about memory leakage here, a bigger concern would be the calling code in does_not_exist_skipping(), which is using NameListToString() which allocates at least 1024 bytes for the name of the non-existent object without freeing it. Fair enough. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 12/1/13, 2:32 AM, Pavel Stehule wrote: trailing whitespace fixed, Peter, what application do you use for this check? git diff --check -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 1 December 2013 07:32, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/11/30 Peter Eisentraut pete...@gmx.net trailing whitespace fixed, Hi, I've been looking at this and I think it's mostly in good shape, but I spotted a few minor issues: * There's a typo in the notice text in a couple of places --- does not exists, skipping should be does not exist, skipping. * In does_not_exist_skipping(), the schema existence checks for extensions and foreign data wrappers are not necessary, since I don't think they can be schema-qualified. * Also in does_not_exist_skipping(), in the block for casts, it is no longer safe to use format_type_be() because it is now possible for the types to not exist at this point. So I think it needs to use TypeNameToString() there instead, otherwise it might raise a no such type ERROR while trying to issue the NOTICE. * In DropErrorMsgNonExistent(), I think the ERROR text should report no such schema in the same way as the NOTICE text when the schema doesn't exist for consistency with the other ERRORs and NOTICEs. * Some more code is needed to make DROP OPERATOR FAMILY IF EXISTS tolerate a non-existent schema. Attached is an updated patch for those issues. I also tried to tidy up the code in dropcmds.c a bit, removing some duplicated code, and making parent_does_not_exist_skipping() have the same signature as schema_does_not_exist_skipping(). This makes the code in does_not_exist_skipping() a little neater, and means that parent_does_not_exist_skipping() can just call schema_does_not_exist_skipping() to check for the existence of the parent relation's schema. I hope those changes are all OK. Regards, Dean drop-if-exists-consistency-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello it looks well, thank you Regards Pavel 2013/12/1 Dean Rasheed dean.a.rash...@gmail.com On 1 December 2013 07:32, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/11/30 Peter Eisentraut pete...@gmx.net trailing whitespace fixed, Hi, I've been looking at this and I think it's mostly in good shape, but I spotted a few minor issues: * There's a typo in the notice text in a couple of places --- does not exists, skipping should be does not exist, skipping. * In does_not_exist_skipping(), the schema existence checks for extensions and foreign data wrappers are not necessary, since I don't think they can be schema-qualified. * Also in does_not_exist_skipping(), in the block for casts, it is no longer safe to use format_type_be() because it is now possible for the types to not exist at this point. So I think it needs to use TypeNameToString() there instead, otherwise it might raise a no such type ERROR while trying to issue the NOTICE. * In DropErrorMsgNonExistent(), I think the ERROR text should report no such schema in the same way as the NOTICE text when the schema doesn't exist for consistency with the other ERRORs and NOTICEs. * Some more code is needed to make DROP OPERATOR FAMILY IF EXISTS tolerate a non-existent schema. Attached is an updated patch for those issues. I also tried to tidy up the code in dropcmds.c a bit, removing some duplicated code, and making parent_does_not_exist_skipping() have the same signature as schema_does_not_exist_skipping(). This makes the code in does_not_exist_skipping() a little neater, and means that parent_does_not_exist_skipping() can just call schema_does_not_exist_skipping() to check for the existence of the parent relation's schema. I hope those changes are all OK. Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Fri, 2013-11-29 at 09:06 +0100, Pavel Stehule wrote: attached patch implement unified behave for DROP IF EXISTS statements as was discussed src/backend/catalog/namespace.c:1743: indent with spaces. src/backend/commands/dropcmds.c:322: indent with spaces. src/backend/commands/dropcmds.c:323: indent with spaces. src/backend/commands/dropcmds.c:331: indent with spaces. src/backend/commands/dropcmds.c:332: indent with spaces. src/backend/commands/tablecmds.c:702: indent with spaces. src/backend/commands/tablecmds.c:859: indent with spaces. src/backend/parser/parse_func.c:1065: trailing whitespace. src/backend/parser/parse_func.c:1066: indent with spaces. src/backend/parser/parse_type.c:60: indent with spaces. src/include/parser/parse_type.h:25: indent with spaces. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Fwd: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/11/30 Peter Eisentraut pete...@gmx.net
trailing whitespace
fixed,
Peter, what application do you use for this check?
Regards
Pavel
commit 88e0a6b97968f88aaa1e3cef17fc2e6e2ca9f25d
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Fri Nov 29 11:10:07 2013 +0100
initial
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 4434dd6..b32f2e4 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -909,7 +909,7 @@ TypeIsVisible(Oid typid)
*/
FuncCandidateList
FuncnameGetCandidates(List *names, int nargs, List *argnames,
- bool expand_variadic, bool expand_defaults)
+ bool expand_variadic, bool expand_defaults, bool noError)
{
FuncCandidateList resultList = NULL;
bool any_special = false;
@@ -928,7 +928,9 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
if (schemaname)
{
/* use exact schema given */
- namespaceId = LookupExplicitNamespace(schemaname, false);
+ namespaceId = LookupExplicitNamespace(schemaname, noError);
+ if (!OidIsValid(namespaceId))
+ return NULL;
}
else
{
@@ -1414,7 +1416,7 @@ FunctionIsVisible(Oid funcid)
visible = false;
clist = FuncnameGetCandidates(list_make1(makeString(proname)),
- nargs, NIL, false, false);
+ nargs, NIL, false, false, false);
for (; clist; clist = clist-next)
{
@@ -1446,7 +1448,7 @@ FunctionIsVisible(Oid funcid)
* namespace search path.
*/
Oid
-OpernameGetOprid(List *names, Oid oprleft, Oid oprright)
+OpernameGetOprid(List *names, Oid oprleft, Oid oprright, bool noError)
{
char *schemaname;
char *opername;
@@ -1462,18 +1464,21 @@ OpernameGetOprid(List *names, Oid oprleft, Oid oprright)
Oid namespaceId;
HeapTuple opertup;
- namespaceId = LookupExplicitNamespace(schemaname, false);
- opertup = SearchSysCache4(OPERNAMENSP,
- CStringGetDatum(opername),
- ObjectIdGetDatum(oprleft),
- ObjectIdGetDatum(oprright),
- ObjectIdGetDatum(namespaceId));
- if (HeapTupleIsValid(opertup))
+ namespaceId = LookupExplicitNamespace(schemaname, noError);
+ if (OidIsValid(namespaceId))
{
- Oid result = HeapTupleGetOid(opertup);
+ opertup = SearchSysCache4(OPERNAMENSP,
+ CStringGetDatum(opername),
+ ObjectIdGetDatum(oprleft),
+ ObjectIdGetDatum(oprright),
+ ObjectIdGetDatum(namespaceId));
+ if (HeapTupleIsValid(opertup))
+ {
+Oid result = HeapTupleGetOid(opertup);
- ReleaseSysCache(opertup);
- return result;
+ReleaseSysCache(opertup);
+return result;
+ }
}
return InvalidOid;
}
@@ -1734,7 +1739,8 @@ OperatorIsVisible(Oid oprid)
char *oprname = NameStr(oprform-oprname);
visible = (OpernameGetOprid(list_make1(makeString(oprname)),
- oprform-oprleft, oprform-oprright)
+ oprform-oprleft, oprform-oprright,
+ false)
== oprid);
}
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 9011190..6a851fa 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -580,8 +580,8 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
{
TypeName *sourcetype = (TypeName *) linitial(objname);
TypeName *targettype = (TypeName *) linitial(objargs);
- Oid sourcetypeid = typenameTypeId(NULL, sourcetype);
- Oid targettypeid = typenameTypeId(NULL, targettype);
+ Oid sourcetypeid = LookupTypeNameOid(NULL, sourcetype, missing_ok);
+ Oid targettypeid = LookupTypeNameOid(NULL, targettype, missing_ok);
address.classId = CastRelationId;
address.objectId =
@@ -942,26 +942,30 @@ get_object_address_relobject(ObjectType objtype, List *objname,
/* Extract relation name and open relation. */
relname = list_truncate(list_copy(objname), nnames - 1);
- relation = heap_openrv(makeRangeVarFromNameList(relname),
- AccessShareLock);
- reloid = RelationGetRelid(relation);
+ relation = heap_openrv_extended(makeRangeVarFromNameList(relname),
+ AccessShareLock,
+ missing_ok);
+
+ reloid = (relation != NULL) ? RelationGetRelid(relation) : InvalidOid;
switch (objtype)
{
case OBJECT_RULE:
address.classId = RewriteRelationId;
-address.objectId = get_rewrite_oid(reloid, depname, missing_ok);
+address.objectId = OidIsValid(reloid) ?
+ get_rewrite_oid(reloid, depname, missing_ok) : InvalidOid;
address.objectSubId = 0;
break;
case OBJECT_TRIGGER:
address.classId = TriggerRelationId;
-address.objectId = get_trigger_oid(reloid, depname, missing_ok);
+address.objectId = OidIsValid(reloid) ?
+ get_trigger_oid(reloid, depname, missing_ok) : InvalidOid;
address.objectSubId = 0;
break;
case OBJECT_CONSTRAINT:
address.classId = ConstraintRelationId;
-address.objectId =
- get_relation_constraint_oid(reloid, depname,
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
I'll prepare patch 2013/11/27 Tom Lane t...@sss.pgh.pa.us Dean Rasheed dean.a.rash...@gmail.com writes: Actually the IF EXISTS in DROP TABLE now applies to the schema as well. Unfortunately there is currently no consistency across the various DROP commands --- some tolerate a non-existent schema, while others error out. Yeah. I think now that we've had this discussion, we should make them all tolerate a non-existent schema. I'm fine with having that happen over a series of patches rather than all at once though. Also amongst those that tolerate a non-existent schema, the resulting notices are not consistent --- some report the schema-qualified object name, while others just report the local object name. Less excited about this part, but on the whole I'd vote for the schema no_such_schema does not exist wording in cases where the schema isn't there. The other way is less specific for no very good reason. regards, tom lane
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello
2013/11/27 Tom Lane t...@sss.pgh.pa.us
Dean Rasheed dean.a.rash...@gmail.com writes:
Actually the IF EXISTS in DROP TABLE now applies to the schema as
well. Unfortunately there is currently no consistency across the
various DROP commands --- some tolerate a non-existent schema, while
others error out.
Yeah. I think now that we've had this discussion, we should make them
all tolerate a non-existent schema. I'm fine with having that happen
over a series of patches rather than all at once though.
Also amongst those that tolerate a non-existent
schema, the resulting notices are not consistent --- some report the
schema-qualified object name, while others just report the local
object name.
Less excited about this part, but on the whole I'd vote for the schema
no_such_schema does not exist wording in cases where the schema isn't
there. The other way is less specific for no very good reason.
can be used this behave (see attached patch, please)?
if it is correct, I'll work on second patch, that unify check and notices
for other DROP IF EXISTS statements.
Regards
Pavel
regards, tom lane
commit a2de2b402247fded7feba90bf299e91ee14aacca
Author: Pavel Stehule pavel.steh...@gooddata.com
Date: Thu Nov 28 08:39:43 2013 +0100
initial
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 9011190..234673f 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -942,26 +942,30 @@ get_object_address_relobject(ObjectType objtype, List *objname,
/* Extract relation name and open relation. */
relname = list_truncate(list_copy(objname), nnames - 1);
- relation = heap_openrv(makeRangeVarFromNameList(relname),
- AccessShareLock);
- reloid = RelationGetRelid(relation);
+ relation = heap_openrv_extended(makeRangeVarFromNameList(relname),
+ AccessShareLock,
+ missing_ok);
+
+ reloid = (relation != NULL) ? RelationGetRelid(relation) : InvalidOid;
switch (objtype)
{
case OBJECT_RULE:
address.classId = RewriteRelationId;
-address.objectId = get_rewrite_oid(reloid, depname, missing_ok);
+address.objectId = OidIsValid(reloid) ?
+ get_rewrite_oid(reloid, depname, missing_ok) : InvalidOid;
address.objectSubId = 0;
break;
case OBJECT_TRIGGER:
address.classId = TriggerRelationId;
-address.objectId = get_trigger_oid(reloid, depname, missing_ok);
+address.objectId = OidIsValid(reloid) ?
+ get_trigger_oid(reloid, depname, missing_ok) : InvalidOid;
address.objectSubId = 0;
break;
case OBJECT_CONSTRAINT:
address.classId = ConstraintRelationId;
-address.objectId =
- get_relation_constraint_oid(reloid, depname, missing_ok);
+address.objectId = OidIsValid(reloid) ?
+ get_relation_constraint_oid(reloid, depname, missing_ok) : InvalidOid;
address.objectSubId = 0;
break;
default:
@@ -975,7 +979,9 @@ get_object_address_relobject(ObjectType objtype, List *objname,
/* Avoid relcache leak when object not found. */
if (!OidIsValid(address.objectId))
{
- heap_close(relation, AccessShareLock);
+ if (relation != NULL)
+heap_close(relation, AccessShareLock);
+
relation = NULL; /* department of accident prevention */
return address;
}
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index b32ad3a..191aa17 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -23,6 +23,7 @@
#include catalog/pg_class.h
#include catalog/pg_proc.h
#include commands/defrem.h
+#include lib/stringinfo.h
#include miscadmin.h
#include nodes/makefuncs.h
#include parser/parse_type.h
@@ -32,6 +33,12 @@
static void does_not_exist_skipping(ObjectType objtype,
List *objname, List *objargs);
+static void prepare_notice_recheck_parent(const char *message,
+ List *objname,
+const char **msg,
+char **name,
+char **args);
+
/*
* Drop one or more objects.
*
@@ -125,6 +132,53 @@ RemoveObjects(DropStmt *stmt)
}
/*
+ * Prepare text of does not exist, skipping message for triggers and rules,
+ * where we check schema name first, then table name, and as last we raise
+ * message about missing object.
+ */
+static void
+prepare_notice_recheck_parent(const char *message, List *objname,
+ const char **msg,
+ char **name,
+ char **args)
+{
+ RangeVar *rel;
+
+ rel = makeRangeVarFromNameList(list_truncate(list_copy(objname),
+ list_length(objname) - 1));
+
+ if (rel-schemaname != NULL !OidIsValid(LookupNamespaceNoError(rel-schemaname)))
+ {
+ *msg = gettext_noop(schema \%s\ does not exists, skipping);
+ *name = rel-schemaname;
+ *args = NULL;
+ }
+ else if (!OidIsValid(RangeVarGetRelid(rel, NoLock, true)))
+ {
+ *msg = gettext_noop(relation \%s\ does not exists, skipping);
+ *name = rel-relname;
+ *args = NULL;
+ }
+ else
+ {
+
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 11/24/13, 2:28 PM, Pavel Stehule wrote: Note: DROP TRIGGER ON tablename is PostgreSQL feature - no other databases (without PostgreSQL forks) uses this syntax - so we don't need thinking what is in (or what will be) in ANSI standard (or what other databases does). In this moment syntax of DROP TRIGGER is non standard. So if we can adopt design (idea) in SQL anywhere or MySQL, then DROP TRIGGER IF EXISTS should be enough. In our implementation there are two conditions, but we should not to check if target table exists (from statement purpose). Right, we might as well consider 'trigger ON tablename' to be the full name of the trigger and just treat it like a unit. But then a single IF EXISTS clause is still inconsistent with DROP TABLE nonexistent.foo, which fails if the schema does not exist. In other words, the IF EXISTS clause only applies to the end of an name chain. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 26 November 2013 19:54, Peter Eisentraut pete...@gmx.net wrote: On 11/24/13, 2:28 PM, Pavel Stehule wrote: Note: DROP TRIGGER ON tablename is PostgreSQL feature - no other databases (without PostgreSQL forks) uses this syntax - so we don't need thinking what is in (or what will be) in ANSI standard (or what other databases does). In this moment syntax of DROP TRIGGER is non standard. So if we can adopt design (idea) in SQL anywhere or MySQL, then DROP TRIGGER IF EXISTS should be enough. In our implementation there are two conditions, but we should not to check if target table exists (from statement purpose). Right, we might as well consider 'trigger ON tablename' to be the full name of the trigger and just treat it like a unit. Yeah, that's how I would view it. But then a single IF EXISTS clause is still inconsistent with DROP TABLE nonexistent.foo, which fails if the schema does not exist. In other words, the IF EXISTS clause only applies to the end of an name chain. Actually the IF EXISTS in DROP TABLE now applies to the schema as well. Unfortunately there is currently no consistency across the various DROP commands --- some tolerate a non-existent schema, while others error out. Also amongst those that tolerate a non-existent schema, the resulting notices are not consistent --- some report the schema-qualified object name, while others just report the local object name. Here is the current state of HEAD: DROP AGGREGATE IF EXISTS no_such_schema.foo(int); ERROR: schema no_such_schema does not exist DROP CAST IF EXISTS (no_such_schema.foo AS no_such_schema.bar); ERROR: schema no_such_schema does not exist DROP COLLATION IF EXISTS no_such_schema.foo; NOTICE: collation no_such_schema.foo does not exist, skipping DROP COLLATION DROP CONVERSION IF EXISTS no_such_schema.foo; NOTICE: conversion no_such_schema.foo does not exist, skipping DROP CONVERSION DROP DOMAIN IF EXISTS no_such_schema.foo; ERROR: schema no_such_schema does not exist DROP FOREIGN TABLE IF EXISTS no_such_schema.foo; NOTICE: foreign table foo does not exist, skipping DROP FOREIGN TABLE DROP FUNCTION IF EXISTS no_such_schema.foo(); ERROR: schema no_such_schema does not exist DROP INDEX IF EXISTS no_such_schema.foo; NOTICE: index foo does not exist, skipping DROP INDEX DROP MATERIALIZED VIEW IF EXISTS no_such_schema.foo; NOTICE: materialized view foo does not exist, skipping DROP MATERIALIZED VIEW DROP OPERATOR IF EXISTS no_such_schema.+ (int, int); ERROR: schema no_such_schema does not exist DROP OPERATOR CLASS IF EXISTS no_such_schema.widget_ops USING btree; ERROR: schema no_such_schema does not exist DROP OPERATOR FAMILY IF EXISTS no_such_schema.float_ops USING btree; ERROR: schema no_such_schema does not exist DROP RULE IF EXISTS foo ON no_such_schema.bar; ERROR: schema no_such_schema does not exist DROP SEQUENCE IF EXISTS no_such_schema.foo; NOTICE: sequence foo does not exist, skipping DROP SEQUENCE DROP TABLE IF EXISTS no_such_schema.foo; NOTICE: table foo does not exist, skipping DROP TABLE DROP TEXT SEARCH CONFIGURATION IF EXISTS no_such_schema.foo; NOTICE: text search configuration no_such_schema.foo does not exist, skipping DROP TEXT SEARCH CONFIGURATION DROP TEXT SEARCH DICTIONARY IF EXISTS no_such_schema.foo; NOTICE: text search dictionary no_such_schema.foo does not exist, skipping DROP TEXT SEARCH DICTIONARY DROP TEXT SEARCH PARSER IF EXISTS no_such_schema.foo; NOTICE: text search parser no_such_schema.foo does not exist, skipping DROP TEXT SEARCH PARSER DROP TEXT SEARCH TEMPLATE IF EXISTS no_such_schema.foo; NOTICE: text search template no_such_schema.foo does not exist, skipping DROP TEXT SEARCH TEMPLATE DROP TRIGGER IF EXISTS foo ON no_such_schema.bar; ERROR: schema no_such_schema does not exist DROP TYPE IF EXISTS no_such_schema.foo; ERROR: schema no_such_schema does not exist DROP VIEW IF EXISTS no_such_schema.foo; NOTICE: view foo does not exist, skipping DROP VIEW That's a lot of inconsistency --- 10 errors vs 12 notices (6 with schema-qualified names and 6 with only local names). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Dean Rasheed dean.a.rash...@gmail.com writes: Actually the IF EXISTS in DROP TABLE now applies to the schema as well. Unfortunately there is currently no consistency across the various DROP commands --- some tolerate a non-existent schema, while others error out. Yeah. I think now that we've had this discussion, we should make them all tolerate a non-existent schema. I'm fine with having that happen over a series of patches rather than all at once though. Also amongst those that tolerate a non-existent schema, the resulting notices are not consistent --- some report the schema-qualified object name, while others just report the local object name. Less excited about this part, but on the whole I'd vote for the schema no_such_schema does not exist wording in cases where the schema isn't there. The other way is less specific for no very good reason. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/11/21 Peter Eisentraut pete...@gmx.net On 11/21/13, 2:35 AM, Pavel Stehule wrote: I am feeling, so almost all people prefer DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; Can we live with it? Fine with me. I think it helps if you consider IF EXISTS an attribute of the command, not an attribute of the command parameters. Now we should be aware that this sort of sets a precedent for ALTER TABLE IF EXISTS ... DROP ANYTHING ... and similar composite commands. If might be worth checking other SQL databases. We stole the IF EXISTS from somewhere, I believe. I did some searching: So DROP TRIGGER IF EXISTS is supported by SQL anywhere, MySQL Doesn't support: MS SQL server (conditional drops is by T-SQL IF EXISTS() statement), Oracle, DB2, But significant difference between PostgreSQL and other databases is requirement to specify table in DROP statement. So in SQL anywhere or in MySQL DROP TRIGGER IF EXISTS is fully fault tolerant, there are not possibility to specify table. Note: DROP TRIGGER ON tablename is PostgreSQL feature - no other databases (without PostgreSQL forks) uses this syntax - so we don't need thinking what is in (or what will be) in ANSI standard (or what other databases does). In this moment syntax of DROP TRIGGER is non standard. So if we can adopt design (idea) in SQL anywhere or MySQL, then DROP TRIGGER IF EXISTS should be enough. In our implementation there are two conditions, but we should not to check if target table exists (from statement purpose). So now, +1 for using DROP TRIGGER IF EXISTS name ON tablename without requirement for tablename Regards Pavel
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 11/21/13, 2:35 AM, Pavel Stehule wrote: I am feeling, so almost all people prefer DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; Can we live with it? Fine with me. I think it helps if you consider IF EXISTS an attribute of the command, not an attribute of the command parameters. Now we should be aware that this sort of sets a precedent for ALTER TABLE IF EXISTS ... DROP ANYTHING ... and similar composite commands. If might be worth checking other SQL databases. We stole the IF EXISTS from somewhere, I believe. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-11-21 17:14:17 -0500, Peter Eisentraut wrote: On 11/21/13, 2:35 AM, Pavel Stehule wrote: I am feeling, so almost all people prefer DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; Can we live with it? Fine with me. I think it helps if you consider IF EXISTS an attribute of the command, not an attribute of the command parameters. Now we should be aware that this sort of sets a precedent for ALTER TABLE IF EXISTS ... DROP ANYTHING ... and similar composite commands. That already has 2 independent IF EXISTS, so I think the precedence argument goes the other way round. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello 2013/11/19 Robert Haas robertmh...@gmail.com On Tue, Nov 19, 2013 at 3:53 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: 1). Keep the existing syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; but make it tolerate a non-existent table when IF EXISTS is specified. I don't love this option, but I like it better than the other proposals. we are in agreement, so we want this feature. How we can decide about syntax? I am feeling, so almost all people prefer DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; Can we live with it? Regards Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 12 November 2013 16:00, Pavel Stehule pavel.steh...@gmail.com wrote: Hello here is patch with fault tolerant drop trigger and drop rule support drop trigger [if exists] trgname on [if exists] tablename; drop rule [if exists] trgname on [if exists] tablename; Regards Pavel Hi, I have just started looking at this patch. It applies cleanly to head, and appears to work as intended. I have a question though about the syntax. Looking back over this thread, there seem to have been 3 different possibilities discussed: 1). Keep the existing syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; but make it tolerate a non-existent table when IF EXISTS is specified. 2). Support 2 independent levels of IF EXISTS using the syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] There was some consensus for this, but then Pavel pointed out that it is inconsistent with other DROP commands, which all have the IF EXISTS before the object to which it refers. 3). Support 2 independent levels of IF EXISTS using the syntax: DROP TRIGGER [ IF EXISTS ] name ON [ IF EXISTS ] table_name [ CASCADE | RESTRICT ] which is what the latest patch does. The syntax in option (3) is certainly more consistent with other DROP commands, but it feels pretty clunky from a grammar point-of-view. It also feels overly complex for the use cases discussed. Personally I would prefer option (1). The SQL standard syntax is simply DROP TRIGGER name. The only reason we have the ON table_name part is that our trigger names aren't globally unique, so trigger_name ON table_name is required to uniquely identify the trigger to drop, which would seem to be directly analogous to specifying a schema in DROP TABLE, and we've already made that tolerate a non-existent schema if IF EXISTS is used. This seems rather different from ALTER TABLE, which allows multiple sub-commands on the same table, so naturally lends itself to multiple independent DROP objtype [IF EXISTS] sub-commands underneath the top-level ALTER TABLE [IF EXISTS], for example: ALTER TABLE IF EXISTS table_name DROP COLUMN IF EXISTS col_name, DROP CONSTRAINT IF EXISTS constr_name; So what we currently have can be summarised as 2 classes of commands/sub-commands to which IF EXISTS applies: ALTER objtype [IF EXISTS] ... DROP objtype [IF EXISTS] ... We don't yet have multiple levels of IF EXISTS within the same DROP, and I don't think it is necessary. For example, no one seems to be asking for DROP TABLE [IF EXISTS] table_name IN [IF EXISTS] schema_name Anyway, that's just my opinion. Clearly there is at least one person with a different opinion. What do other people think? Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello I am thinking so @2 is not good idea. Using well known idiom IF EXISTS once before table name and second after table name can be difficult and messy for users. If you like it, use different idiom or different keyword, please. My person favourite is @1 - fault tolerant version - but I understand to objection (what was reason, why I wrote a last version @3) - @1 and @3 are good decision. Regards Pavel 2013/11/19 Dean Rasheed dean.a.rash...@gmail.com On 12 November 2013 16:00, Pavel Stehule pavel.steh...@gmail.com wrote: Hello here is patch with fault tolerant drop trigger and drop rule support drop trigger [if exists] trgname on [if exists] tablename; drop rule [if exists] trgname on [if exists] tablename; Regards Pavel Hi, I have just started looking at this patch. It applies cleanly to head, and appears to work as intended. I have a question though about the syntax. Looking back over this thread, there seem to have been 3 different possibilities discussed: 1). Keep the existing syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; but make it tolerate a non-existent table when IF EXISTS is specified. 2). Support 2 independent levels of IF EXISTS using the syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] There was some consensus for this, but then Pavel pointed out that it is inconsistent with other DROP commands, which all have the IF EXISTS before the object to which it refers. 3). Support 2 independent levels of IF EXISTS using the syntax: DROP TRIGGER [ IF EXISTS ] name ON [ IF EXISTS ] table_name [ CASCADE | RESTRICT ] which is what the latest patch does. The syntax in option (3) is certainly more consistent with other DROP commands, but it feels pretty clunky from a grammar point-of-view. It also feels overly complex for the use cases discussed. Personally I would prefer option (1). The SQL standard syntax is simply DROP TRIGGER name. The only reason we have the ON table_name part is that our trigger names aren't globally unique, so trigger_name ON table_name is required to uniquely identify the trigger to drop, which would seem to be directly analogous to specifying a schema in DROP TABLE, and we've already made that tolerate a non-existent schema if IF EXISTS is used. This seems rather different from ALTER TABLE, which allows multiple sub-commands on the same table, so naturally lends itself to multiple independent DROP objtype [IF EXISTS] sub-commands underneath the top-level ALTER TABLE [IF EXISTS], for example: ALTER TABLE IF EXISTS table_name DROP COLUMN IF EXISTS col_name, DROP CONSTRAINT IF EXISTS constr_name; So what we currently have can be summarised as 2 classes of commands/sub-commands to which IF EXISTS applies: ALTER objtype [IF EXISTS] ... DROP objtype [IF EXISTS] ... We don't yet have multiple levels of IF EXISTS within the same DROP, and I don't think it is necessary. For example, no one seems to be asking for DROP TABLE [IF EXISTS] table_name IN [IF EXISTS] schema_name Anyway, that's just my opinion. Clearly there is at least one person with a different opinion. What do other people think? Regards, Dean
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Tue, Nov 19, 2013 at 3:53 AM, Dean Rasheed dean.a.rash...@gmail.com wrote: 1). Keep the existing syntax: DROP TRIGGER [ IF EXISTS ] name ON table_name [ CASCADE | RESTRICT ]; but make it tolerate a non-existent table when IF EXISTS is specified. I don't love this option, but I like it better than the other proposals. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello
here is patch with fault tolerant drop trigger and drop rule support
drop trigger [if exists] trgname on [if exists] tablename;
drop rule [if exists] trgname on [if exists] tablename;
Regards
Pavel
2013/11/11 Pavel Stehule pavel.steh...@gmail.com
2013/11/11 Tom Lane t...@sss.pgh.pa.us
Andres Freund and...@2ndquadrant.com writes:
Turns out that's bogus - ALTER TABLE has two levels of NOT EXISTS.
Maybe we should just do the same for DROP TRIGGER?
DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE |
RESTRICT ]
Works for me.
for me too
tomorrow I'll prepare patch
Regards
Pavel
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
diff --git a/doc/src/sgml/ref/drop_rule.sgml b/doc/src/sgml/ref/drop_rule.sgml
index c845872..585f67b 100644
--- a/doc/src/sgml/ref/drop_rule.sgml
+++ b/doc/src/sgml/ref/drop_rule.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-DROP RULE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON replaceable class=PARAMETERtable_name/replaceable [ CASCADE | RESTRICT ]
+DROP RULE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON [ IF EXISTS ] replaceable class=PARAMETERtable_name/replaceable [ CASCADE | RESTRICT ]
/synopsis
/refsynopsisdiv
@@ -42,8 +42,8 @@ DROP RULE [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON re
termliteralIF EXISTS/literal/term
listitem
para
- Do not throw an error if the rule does not exist. A notice is issued
- in this case.
+ Do not throw an error if the rule does not exist (or if a parent table
+ does not exist). A notice is issued in this case.
/para
/listitem
/varlistentry
diff --git a/doc/src/sgml/ref/drop_trigger.sgml b/doc/src/sgml/ref/drop_trigger.sgml
index 3ec6cc7..1f46eff 100644
--- a/doc/src/sgml/ref/drop_trigger.sgml
+++ b/doc/src/sgml/ref/drop_trigger.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-DROP TRIGGER [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON replaceable class=PARAMETERtable_name/replaceable [ CASCADE | RESTRICT ]
+DROP TRIGGER [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON [ IF EXISTS ] replaceable class=PARAMETERtable_name/replaceable [ CASCADE | RESTRICT ]
/synopsis
/refsynopsisdiv
@@ -44,8 +44,8 @@ DROP TRIGGER [ IF EXISTS ] replaceable class=PARAMETERname/replaceable ON
termliteralIF EXISTS/literal/term
listitem
para
- Do not throw an error if the trigger does not exist. A notice is issued
- in this case.
+ Do not throw an error if the trigger does not exist (or parent table
+ does not exist). A notice is issued in this case.
/para
/listitem
/varlistentry
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index cecddf1..36ef9ae 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -423,7 +423,8 @@ static ObjectAddress get_relation_by_qualified_name(ObjectType objtype,
List *objname, Relation *relp,
LOCKMODE lockmode, bool missing_ok);
static ObjectAddress get_object_address_relobject(ObjectType objtype,
- List *objname, Relation *relp, bool missing_ok);
+ List *objname, Relation *relp,
+ bool missing_ok, bool missing_parent_ok);
static ObjectAddress get_object_address_attribute(ObjectType objtype,
List *objname, Relation *relp,
LOCKMODE lockmode, bool missing_ok);
@@ -464,7 +465,8 @@ static void getRelationIdentity(StringInfo buffer, Oid relid);
*/
ObjectAddress
get_object_address(ObjectType objtype, List *objname, List *objargs,
- Relation *relp, LOCKMODE lockmode, bool missing_ok)
+ Relation *relp, LOCKMODE lockmode,
+ bool missing_ok, bool missing_parent_ok)
{
ObjectAddress address;
ObjectAddress old_address = {InvalidOid, InvalidOid, 0};
@@ -507,7 +509,9 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
case OBJECT_TRIGGER:
case OBJECT_CONSTRAINT:
address = get_object_address_relobject(objtype, objname,
- relation, missing_ok);
+ relation,
+ missing_ok,
+ missing_parent_ok);
break;
case OBJECT_DATABASE:
case OBJECT_EXTENSION:
@@ -622,7 +626,7 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
*/
if (!OidIsValid(address.objectId))
{
- Assert(missing_ok);
+ Assert(missing_ok || missing_parent_ok);
return address;
}
@@ -898,7 +902,9 @@ get_relation_by_qualified_name(ObjectType objtype, List *objname,
*/
static ObjectAddress
get_object_address_relobject(ObjectType objtype, List *objname,
- Relation *relp, bool missing_ok)
+ Relation *relp,
+ bool
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
I can agree, so DROP TRIGGER doesn't need a IF EXISTS clause when it is executed after DROP TABLE. pg_dump -c produces: DROP TRIGGER jjj ON public.foo; DROP TABLE public.foo; DROP FUNCTION public.f1(); DROP EXTENSION plpgsql; DROP SCHEMA public; Is there some reason why we use explicitly DROP TRIGGER there? Regards Pavel 2013/10/15 Andres Freund and...@2ndquadrant.com On 2013-10-15 00:23:15 +0200, Tomas Vondra wrote: Hi, On 14.10.2013 23:44, Andres Freund wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Not sure I agree with your reasoning. Isn't that equally true for 'IF EXISTS' clause with all commands in general? Why should we use likely to hide typos argument in this case and not the others? Because there simply is no reason to issue a DROP TRIGGER IF EXISTS if you don't need the contents of the table. In that case you can just issue a DROP TABLE IF EXISTS and start anew. The purpose of this patch was to add support for quiet pg_restore --clean and pg_restore should not do typos (if it does, we're in much deeper troubles I guess). Why does that even have to do anything for triggers? Emitting DROP TABLE should be enough. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/11/11 Tom Lane t...@sss.pgh.pa.us Andres Freund and...@2ndquadrant.com writes: Turns out that's bogus - ALTER TABLE has two levels of NOT EXISTS. Maybe we should just do the same for DROP TRIGGER? DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] Works for me. for me too tomorrow I'll prepare patch Regards Pavel regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/11/11 Pavel Stehule pavel.steh...@gmail.com 2013/11/11 Tom Lane t...@sss.pgh.pa.us Andres Freund and...@2ndquadrant.com writes: Turns out that's bogus - ALTER TABLE has two levels of NOT EXISTS. Maybe we should just do the same for DROP TRIGGER? DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] This syntax is not consistent with other IF EXISTS. should be (IF EXISTS is before name always) DROP TRIGGER [ IF EXISTS ] name ON [ IF EXISTS ] table_name [ CASCADE | RESTRICT ] What do you think about? Regards Pavel Works for me. for me too tomorrow I'll prepare patch Regards Pavel regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
[ catching up on old email ] Andres Freund and...@2ndquadrant.com writes: On 2013-10-15 00:23:15 +0200, Tomas Vondra wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Because there simply is no reason to issue a DROP TRIGGER IF EXISTS if you don't need the contents of the table. In that case you can just issue a DROP TABLE IF EXISTS and start anew. I think this is nonsense. It's only one step removed from why do you need IF EXISTS at all, you should know whether the object is there. The entire point of this syntax is to not need to do detailed analysis about whether the object is there. The pg_dump --clean use-case is sufficient refutation for that, IMO. You're suggesting that pg_dump should make a special case out of what it emits for cleaning a trigger; which we could do I guess, but it would be ugly and fragile. For instance, the special case would probably soon grow some warts for partial-dump scenarios. Anyway, pg_dump is not the only tool that might wish to use DROP IF EXISTS. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-11-10 16:28:27 -0500, Tom Lane wrote: [ catching up on old email ] Andres Freund and...@2ndquadrant.com writes: On 2013-10-15 00:23:15 +0200, Tomas Vondra wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Because there simply is no reason to issue a DROP TRIGGER IF EXISTS if you don't need the contents of the table. In that case you can just issue a DROP TABLE IF EXISTS and start anew. I think this is nonsense. It's only one step removed from why do you need IF EXISTS at all, you should know whether the object is there. The entire point of this syntax is to not need to do detailed analysis about whether the object is there. Well, in my opinion the IF EXISTS refers to the object type being dropped. I.e. with DROP TABLE it refers to the table not existing, with DROP TRIGGER it refers to the trigger not existing. Note how we also error out if you do something like: ALTER TABLE nonexistant DROP COLUMN IF EXISTS bar; The pg_dump --clean use-case is sufficient refutation for that, IMO. You're suggesting that pg_dump should make a special case out of what it emits for cleaning a trigger; which we could do I guess, but it would be ugly and fragile. For instance, the special case would probably soon grow some warts for partial-dump scenarios. ISTM the only way to get a DROP TRIGGER that actually is required is a --section=post-data dump. In the other cases it will get dropped by the DROP TABLE that's executed shortly afterwards. But in that case we'll currently error out during the CREATE TRIGGER shortly afterwards if the table doesn't exist... Maybe the way to fix this properly is to not drop post-data objects that are implicitly dropped by the object that owns them. Anyway, pg_dump is not the only tool that might wish to use DROP IF EXISTS. Well, I am absolutely not arguing against DROP TRIGGER IF NOT EXISTS (which we already have), just against it ignoring nonexistant tables instead of just nonexistant triggers. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Andres Freund and...@2ndquadrant.com writes: On 2013-11-10 16:28:27 -0500, Tom Lane wrote: I think this is nonsense. It's only one step removed from why do you need IF EXISTS at all, you should know whether the object is there. The entire point of this syntax is to not need to do detailed analysis about whether the object is there. Well, in my opinion the IF EXISTS refers to the object type being dropped. I.e. with DROP TABLE it refers to the table not existing, with DROP TRIGGER it refers to the trigger not existing. Then I take it you also think we should undo the changes that made DROP TABLE IF EXISTS foo.bar not fail if schema foo doesn't exist? Because after all, the schema is not the object being dropped. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-11-10 18:16:16 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-11-10 16:28:27 -0500, Tom Lane wrote: I think this is nonsense. It's only one step removed from why do you need IF EXISTS at all, you should know whether the object is there. The entire point of this syntax is to not need to do detailed analysis about whether the object is there. Well, in my opinion the IF EXISTS refers to the object type being dropped. I.e. with DROP TABLE it refers to the table not existing, with DROP TRIGGER it refers to the trigger not existing. Then I take it you also think we should undo the changes that made DROP TABLE IF EXISTS foo.bar not fail if schema foo doesn't exist? Because after all, the schema is not the object being dropped. No, not the same thing imo, although I find that change debatable. Anyway, if we're going to change DROP TRIGGER at the very least ALTER TABLE ... DROP CONSTRAINT also needs to be changed, otherwise we'll gain nothing. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Andres Freund and...@2ndquadrant.com writes: On 2013-11-10 18:16:16 -0500, Tom Lane wrote: Then I take it you also think we should undo the changes that made DROP TABLE IF EXISTS foo.bar not fail if schema foo doesn't exist? Because after all, the schema is not the object being dropped. No, not the same thing imo, although I find that change debatable. Anyway, if we're going to change DROP TRIGGER at the very least ALTER TABLE ... DROP CONSTRAINT also needs to be changed, otherwise we'll gain nothing. That would be a plausible next step, but I don't have a problem with this patch not solving every case like this at once. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-11-10 18:26:26 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-11-10 18:16:16 -0500, Tom Lane wrote: Then I take it you also think we should undo the changes that made DROP TABLE IF EXISTS foo.bar not fail if schema foo doesn't exist? Because after all, the schema is not the object being dropped. No, not the same thing imo, although I find that change debatable. Anyway, if we're going to change DROP TRIGGER at the very least ALTER TABLE ... DROP CONSTRAINT also needs to be changed, otherwise we'll gain nothing. That would be a plausible next step, but I don't have a problem with this patch not solving every case like this at once. Well, there are relatively few tables without primary keys around that have triggers. The dumps currently look like: DROP TRIGGER foo ON public.foo; ALTER TABLE ONLY public.foo DROP CONSTRAINT foo_pkey; ALTER TABLE public.foo ALTER COLUMN id DROP DEFAULT; DROP SEQUENCE public.foo_id_seq; DROP TABLE public.foo; So we'd get approximately one line further unless we fix this for DROP DEFAULT and DROP CONSTRAINT as well. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Andres Freund and...@2ndquadrant.com writes: ... So we'd get approximately one line further unless we fix this for DROP DEFAULT and DROP CONSTRAINT as well. True. As far as pg_dump --clean is concerned, it'd undoubtedly be easier if we did what you suggest and just eliminate the emitted DROP commands for table components, relying on the assumption that there'll never be a partial-dump mode that would allow dumping a table's components without the table. However, the server-side approach has the benefit that it'll likely make life easier for other applications besides pg_dump. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-11-10 18:42:11 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: ... So we'd get approximately one line further unless we fix this for DROP DEFAULT and DROP CONSTRAINT as well. Turns out that's bogus - ALTER TABLE has two levels of NOT EXISTS. Maybe we should just do the same for DROP TRIGGER? DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] However, the server-side approach has the benefit that it'll likely make life easier for other applications besides pg_dump. I am unconvinced that that's the case when using the existing IF EXISTS for DROP TRIGGER, but my complaints would be completely addressed by making it a separate flag. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Andres Freund and...@2ndquadrant.com writes: Turns out that's bogus - ALTER TABLE has two levels of NOT EXISTS. Maybe we should just do the same for DROP TRIGGER? DROP TRIGGER [ IF EXISTS ] name ON table_name [ IF EXISTS ] [ CASCADE | RESTRICT ] Works for me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Tue, Oct 22, 2013 at 9:37 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote: Also, Pavel, this patch is still listed as 'Needs Review' in the CF app, but I haven't seen a response to the concerns in my last message. It looks like this patch has been imported into the 2013-11 CF [1] and marked Needs Review. I looked at the version of the patch pointed to in that CF entry back in July, and noted [2] several problems that still seemed to be present in the patch, for which I never saw a followup from Pavel. IMO this patch should have gotten marked Returned with Feedback pending a response from Pavel. That sounds reasonable to me. Perhaps you should so mark it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/10/24 Robert Haas robertmh...@gmail.com On Tue, Oct 22, 2013 at 9:37 PM, Josh Kupershmidt schmi...@gmail.com wrote: On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote: Also, Pavel, this patch is still listed as 'Needs Review' in the CF app, but I haven't seen a response to the concerns in my last message. It looks like this patch has been imported into the 2013-11 CF [1] and marked Needs Review. I looked at the version of the patch pointed to in that CF entry back in July, and noted [2] several problems that still seemed to be present in the patch, for which I never saw a followup from Pavel. IMO this patch should have gotten marked Returned with Feedback pending a response from Pavel. That sounds reasonable to me. Perhaps you should so mark it. +1 Regards Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt schmi...@gmail.com wrote: Also, Pavel, this patch is still listed as 'Needs Review' in the CF app, but I haven't seen a response to the concerns in my last message. It looks like this patch has been imported into the 2013-11 CF [1] and marked Needs Review. I looked at the version of the patch pointed to in that CF entry back in July, and noted [2] several problems that still seemed to be present in the patch, for which I never saw a followup from Pavel. IMO this patch should have gotten marked Returned with Feedback pending a response from Pavel. Josh [1] https://commitfest.postgresql.org/action/patch_view?id=1174 [2] http://www.postgresql.org/message-id/CAK3UJREth9DVL5U7ewOLQYhXF7EcV5BABFE+pzPQjkPfqbW=v...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Thu, Oct 10, 2013 at 12:54 PM, Andrew Dunstan and...@dunslane.net wrote: This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. +1 from me as well. Also, Pavel, this patch is still listed as 'Needs Review' in the CF app, but I haven't seen a response to the concerns in my last message. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 10/14/2013 05:44 PM, Andres Freund wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. So if I say DROP TRIGGER IF EXISTS foo ON bar; you're ok with succeeding if foo is a typo, but not if bar is? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-10-14 17:59:25 -0400, Andrew Dunstan wrote: On 10/14/2013 05:44 PM, Andres Freund wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. So if I say DROP TRIGGER IF EXISTS foo ON bar; you're ok with succeeding if foo is a typo, but not if bar is? Yes. Normally you won't do DROP TRIGGER if you don't need the table in the first place, in that case you can just DROP TABLE. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hi, On 14.10.2013 23:44, Andres Freund wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Not sure I agree with your reasoning. Isn't that equally true for 'IF EXISTS' clause with all commands in general? Why should we use likely to hide typos argument in this case and not the others? Or do you object only to extending DROP TRIGGER IF EXISTS to if table and trigger exists? It seems natural to me that no table = no trigger so I'm fine with this interpretation, just like Andrew. The purpose of this patch was to add support for quiet pg_restore --clean and pg_restore should not do typos (if it does, we're in much deeper troubles I guess). If you're concerned about users doing typos, they may as well do typos in the trigger name with exactly the same result (trigger not dropped without any kind of error message). I see no reason to support DROP TRIGGER IF EXISTS but restrict the IF EXISTS clause only to the trigger on the grounds of typos. So +1 from me, both to the patch and graceful handling of missing table. kind regards Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 2013-10-15 00:23:15 +0200, Tomas Vondra wrote: Hi, On 14.10.2013 23:44, Andres Freund wrote: On 2013-10-10 12:54:23 -0400, Andrew Dunstan wrote: On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. -1, this seems to likely to just hide typos. Not sure I agree with your reasoning. Isn't that equally true for 'IF EXISTS' clause with all commands in general? Why should we use likely to hide typos argument in this case and not the others? Because there simply is no reason to issue a DROP TRIGGER IF EXISTS if you don't need the contents of the table. In that case you can just issue a DROP TABLE IF EXISTS and start anew. The purpose of this patch was to add support for quiet pg_restore --clean and pg_restore should not do typos (if it does, we're in much deeper troubles I guess). Why does that even have to do anything for triggers? Emitting DROP TABLE should be enough. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On 09/19/2013 06:12 PM, Pavel Stehule wrote: 2013/9/16 Satoshi Nagayasu sn...@uptime.jp mailto:sn...@uptime.jp I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. This thread seems to have gone cold, but I'm inclined to agree with Pavel. If the table doesn't exist, neither does the trigger, and the whole point of the 'IF EXISTS' variants is to provide the ability to issue DROP commands that don't fail if their target is missing. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/9/16 Satoshi Nagayasu sn...@uptime.jp (2013/07/06 1:16), Pavel Stehule wrote: I am sending a patch that removes strict requirements for DROP IF EXISTS statements. This behave is similar to our ALTER IF EXISTS behave now. postgres=# DROP CAST IF EXISTS (sss AS public.casttesttype); NOTICE: types sss and public.casttesttype does not exist, skipping DROP CAST postgres=# DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); NOTICE: function public.pt_in_widget(point,**widget) does not exist, skipping DROP FUNCTION postgres=# DROP OPERATOR IF EXISTS public.% (point, widget); NOTICE: operator public.% does not exist, skipping DROP OPERATOR postgres=# DROP TRIGGER test_trigger_exists ON no_such_table; ERROR: relation no_such_table does not exist postgres=# DROP TRIGGER IF EXISTS test_trigger_exists ON no_such_table; NOTICE: trigger test_trigger_exists for table no_such_table does not exist, skipping DROP TRIGGER This functionality is necessary for correct quite reload from dump without possible warnings I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? My opinion is so, both variants should be ignored - it should be fully fault tolerant in this use case. Regards Pavel I've not understood the pg_restore issue precisely so far, but IMHO DROP TRIGGER IF EXISTS means if the _trigger_ exists, not if the _table_ exists. Is this a correct and/or an expected behavior? Sorry if I missed some consensus which we already made. Any comments? Regards, -- Satoshi Nagayasu sn...@uptime.jp Uptime Technologies, LLC. http://www.uptime.jp
Re: [HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
(2013/07/06 1:16), Pavel Stehule wrote: I am sending a patch that removes strict requirements for DROP IF EXISTS statements. This behave is similar to our ALTER IF EXISTS behave now. postgres=# DROP CAST IF EXISTS (sss AS public.casttesttype); NOTICE: types sss and public.casttesttype does not exist, skipping DROP CAST postgres=# DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); NOTICE: function public.pt_in_widget(point,widget) does not exist, skipping DROP FUNCTION postgres=# DROP OPERATOR IF EXISTS public.% (point, widget); NOTICE: operator public.% does not exist, skipping DROP OPERATOR postgres=# DROP TRIGGER test_trigger_exists ON no_such_table; ERROR: relation no_such_table does not exist postgres=# DROP TRIGGER IF EXISTS test_trigger_exists ON no_such_table; NOTICE: trigger test_trigger_exists for table no_such_table does not exist, skipping DROP TRIGGER This functionality is necessary for correct quite reload from dump without possible warnings I'm looking at this patch, and I have a question here. Should DROP TRIGGER IF EXISTS ignore error for non-existing trigger and non-existing table? Or just only for non-existing trigger? I've not understood the pg_restore issue precisely so far, but IMHO DROP TRIGGER IF EXISTS means if the _trigger_ exists, not if the _table_ exists. Is this a correct and/or an expected behavior? Sorry if I missed some consensus which we already made. Any comments? Regards, -- Satoshi Nagayasu sn...@uptime.jp Uptime Technologies, LLC. http://www.uptime.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I am sending a patch that removes strict requirements for DROP IF EXISTS statements. This behave is similar to our ALTER IF EXISTS behave now. +1 for this idea. But this patch should be treated as a separate issue from the use of IF EXISTS in pg_dump/pg_restore, right? If so, I suggest starting a new thread about this patch to make reviewing easier. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Yes, I wrote a separate patch for next commitfest. Dne 10.7.2013 16:54 Josh Kupershmidt schmi...@gmail.com napsal(a): On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I am sending a patch that removes strict requirements for DROP IF EXISTS statements. This behave is similar to our ALTER IF EXISTS behave now. +1 for this idea. But this patch should be treated as a separate issue from the use of IF EXISTS in pg_dump/pg_restore, right? If so, I suggest starting a new thread about this patch to make reviewing easier. Josh
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Tue, Jul 2, 2013 at 5:39 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/3/8 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/3/8 Josh Kupershmidt schmi...@gmail.com: Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) no some people - like we in our company would to use this feature for quiet and strict mode for plain text format too. enabling this feature has zero overhead so there are no reason block it anywhere. I'm not sure I understand what you're getting at, but maybe my proposal wasn't so clear. Right now, you can specify --clean along with -Fc to pg_dump, and pg_dump will not complain even though this combination is nonsense. I am proposing that pg_dump error out in this case, i.e. $ pg_dump -Fc --file=test.dump --clean -d test pg_dump: option --clean only valid with plain format dump Although this lack of an error a (IMO) misfeature of existing pg_dump, so if you'd rather leave this issue aside for your patch, that is fine. I tested last patch and I am thinking so this patch has sense for custom format too [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump --clean -t a -Fc postgres dump [postgres@localhost ~]$ psql -c drop table a DROP TABLE [postgres@localhost ~]$ pg_restore --clean -d postgres dump pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 171; 1259 16462 TABLE a postgres pg_restore: [archiver (db)] could not execute query: ERROR: table a does not exist Command was: DROP TABLE public.a; WARNING: errors ignored on restore: 1 [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump --if-exist --clean -t a -Fc postgres dump [postgres@localhost ~]$ psql -c drop table a DROP TABLE [postgres@localhost ~]$ pg_restore --clean -d postgres dump So limit for plain format is not too strict I'm not sure I understand what you're proposing here, but for the record I really don't think it's a good idea to encourage the use of pg_dump -Fc --clean ... Right now, we don't error out on this combination of command-line options, but the --clean is effectively a no-op; you have to tell pg_restore to use --clean. IMO, this is basically how it should be: pg_dump, at least in custom-format, is preserving the contents of your database with the understanding that you will use pg_restore wish to restore from this dump in a variety of possible ways. Putting restrictions like --clean into the custom-format dump file just makes that dump file less useful overall. Although it's still not clear to me why the examples you showed above used *both* `pg_dump -Fc --clean ...` and `pg_restore --clean ...` together. Surely the user should be specifying this preference in only one place? Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Tue, Jul 2, 2013 at 7:47 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello remastered patch still there is a issue with dependencies Several of the issues from my last review [1] seem to still be present in this patch, such as review notes #1 and #4. And as discussed previously, I think that the --clean option belongs solely with pg_restore for custom-format dumps. The way the patch handles this is rather confusing, forcing the user to do: $ pg_dump -Fc --clean --if-exists --file=backup.dump ... and then: $ pg_restore --clean ... backup.dump (without --if-exists) to get the desired behavior. Josh [1] http://www.postgresql.org/message-id/CAK3UJRG__4=+f46xamiqa80f_-bqhjcpfwyp8g8fpspqj-j...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello I am sending a patch that removes strict requirements for DROP IF EXISTS statements. This behave is similar to our ALTER IF EXISTS behave now. postgres=# DROP CAST IF EXISTS (sss AS public.casttesttype); NOTICE: types sss and public.casttesttype does not exist, skipping DROP CAST postgres=# DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); NOTICE: function public.pt_in_widget(point,widget) does not exist, skipping DROP FUNCTION postgres=# DROP OPERATOR IF EXISTS public.% (point, widget); NOTICE: operator public.% does not exist, skipping DROP OPERATOR postgres=# DROP TRIGGER test_trigger_exists ON no_such_table; ERROR: relation no_such_table does not exist postgres=# DROP TRIGGER IF EXISTS test_trigger_exists ON no_such_table; NOTICE: trigger test_trigger_exists for table no_such_table does not exist, skipping DROP TRIGGER This functionality is necessary for correct quite reload from dump without possible warnings Regards Pavel 2013/7/2 Pavel Stehule pavel.steh...@gmail.com: Hello remastered patch still there is a issue with dependencies Regards Pavel Stehule 2013/6/17 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I'll see - please, stay tuned to 9.4 first commitfest Hi Pavel, Just a reminder, I didn't see this patch in the current commitfest. I would be happy to spend some more time reviewing if you wish to pursue the patch. Josh drop-if-exists-no-double-check.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello 2013/3/8 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/3/8 Josh Kupershmidt schmi...@gmail.com: Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) no some people - like we in our company would to use this feature for quiet and strict mode for plain text format too. enabling this feature has zero overhead so there are no reason block it anywhere. I'm not sure I understand what you're getting at, but maybe my proposal wasn't so clear. Right now, you can specify --clean along with -Fc to pg_dump, and pg_dump will not complain even though this combination is nonsense. I am proposing that pg_dump error out in this case, i.e. $ pg_dump -Fc --file=test.dump --clean -d test pg_dump: option --clean only valid with plain format dump Although this lack of an error a (IMO) misfeature of existing pg_dump, so if you'd rather leave this issue aside for your patch, that is fine. I tested last patch and I am thinking so this patch has sense for custom format too [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump --clean -t a -Fc postgres dump [postgres@localhost ~]$ psql -c drop table a DROP TABLE [postgres@localhost ~]$ pg_restore --clean -d postgres dump pg_restore: [archiver (db)] Error while PROCESSING TOC: pg_restore: [archiver (db)] Error from TOC entry 171; 1259 16462 TABLE a postgres pg_restore: [archiver (db)] could not execute query: ERROR: table a does not exist Command was: DROP TABLE public.a; WARNING: errors ignored on restore: 1 [postgres@localhost ~]$ /usr/local/pgsql/bin/pg_dump --if-exist --clean -t a -Fc postgres dump [postgres@localhost ~]$ psql -c drop table a DROP TABLE [postgres@localhost ~]$ pg_restore --clean -d postgres dump So limit for plain format is not too strict Regards Pavel Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello 2013/3/8 Josh Kupershmidt schmi...@gmail.com: [Moving to -hackers] On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule pavel.steh...@gmail.com wrote: so * --conditional-drops replaced by --if-exists Thanks for the fixes, I played around with the patch a bit. I was sort of expecting this example to work (after setting up the regression database with `make installcheck`) pg_dump --clean --if-exists -Fp -d regression --file=regression.sql createdb test psql -v ON_ERROR_STOP=1 --single-transaction -d test -f regression.sql But it fails, first at: ... DROP TRIGGER IF EXISTS tsvectorupdate ON public.test_tsvector; ERROR: relation public.test_tsvector does not exist This seems like a shortcoming of DROP TRIGGER ... IF EXISTS, and it looks like DROP RULE ... IF EXISTS has the same problem. I recall DROP ... IF EXISTS being fixed recently for not to error out if the schema specified for the object does not exist, and ISTM the same arguments could be made in favor of fixing DROP TRIGGER/TABLE ... IF EXISTS not to error out if the table doesn't exist. yes, I am thinking so it is probably best solution. Without it I should to generate a DO statement with necessary conditions. :( I'll prepare patch and proposal. Working further through the dump of the regression database, these also present problems for --clean --if-exists dumps: DROP CAST IF EXISTS (text AS public.casttesttype); ERROR: type public.casttesttype does not exist DROP OPERATOR IF EXISTS public.% (point, widget); ERROR: type widget does not exist DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); ERROR: type widget does not exist I'm not sure whether DROP CAST/OPERATOR/FUNCTION IF EXISTS should be more tolerant of nonexistent types, of if the mess could perhaps be avoided by dump reordering. we can raise a warning instead error ? Regards Pavel Note, this usability problem affects unpatched head as well: pg_dump -Fc -d regression --file=regression.dump pg_restore --clean -1 -d regression regression.dump ... pg_restore: [archiver (db)] could not execute query: ERROR: type widget does not exist Command was: DROP FUNCTION public.widget_out(widget); (The use here is a little different than the first example above, but I would still expect this case to work.) The above problems with IF EXISTS aren't really a problem of the patch per se, but IMO it would be nice to straighten all the issues out together for 9.4. * -- additional check, available only with -c option Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) Some comments on the changes: 1. There is at least one IF EXISTS check missing from pg_dump.c, see for example this statement from a dump of the regression database with --if-exists: ALTER TABLE public.nv_child_2010 DROP CONSTRAINT nv_child_2010_d_check; 2. Shouldn't pg_restore get --if-exists as well? 3. + printf(_( --if-exists don't report error if cleaned object doesn't exist\n)); This help output bleeds just over our de facto 80-character limit. Also contractions seem to be avoided elsewhere. It's a little hard to squeeze a decent explanation into one line, but perhaps: Use IF EXISTS when dropping objects would be better. The sgml changes could use some wordsmithing and grammar fixes. I could clean these up for you in a later version if you'd like. 4. There seem to be spurious whitespace changes to the function prototype and declaration for _printTocEntry. That's all I've had time for so far... Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
Hello remastered patch still there is a issue with dependencies Regards Pavel Stehule 2013/6/17 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I'll see - please, stay tuned to 9.4 first commitfest Hi Pavel, Just a reminder, I didn't see this patch in the current commitfest. I would be happy to spend some more time reviewing if you wish to pursue the patch. Josh conditional-drops.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/6/17 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I'll see - please, stay tuned to 9.4 first commitfest Hi Pavel, Just a reminder, I didn't see this patch in the current commitfest. I would be happy to spend some more time reviewing if you wish to pursue the patch. Hello yes, I hadn't free time for finalization of last patch. I hope so I can do final version next month to next commitfest. Regards and thank you Pavel Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule pavel.steh...@gmail.com wrote: I'll see - please, stay tuned to 9.4 first commitfest Hi Pavel, Just a reminder, I didn't see this patch in the current commitfest. I would be happy to spend some more time reviewing if you wish to pursue the patch. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/3/8 Josh Kupershmidt schmi...@gmail.com: [Moving to -hackers] On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule pavel.steh...@gmail.com wrote: so * --conditional-drops replaced by --if-exists Thanks for the fixes, I played around with the patch a bit. I was sort of expecting this example to work (after setting up the regression database with `make installcheck`) pg_dump --clean --if-exists -Fp -d regression --file=regression.sql createdb test psql -v ON_ERROR_STOP=1 --single-transaction -d test -f regression.sql But it fails, first at: ... DROP TRIGGER IF EXISTS tsvectorupdate ON public.test_tsvector; ERROR: relation public.test_tsvector does not exist This seems like a shortcoming of DROP TRIGGER ... IF EXISTS, and it looks like DROP RULE ... IF EXISTS has the same problem. I recall DROP ... IF EXISTS being fixed recently for not to error out if the schema specified for the object does not exist, and ISTM the same arguments could be made in favor of fixing DROP TRIGGER/TABLE ... IF EXISTS not to error out if the table doesn't exist. Working further through the dump of the regression database, these also present problems for --clean --if-exists dumps: DROP CAST IF EXISTS (text AS public.casttesttype); ERROR: type public.casttesttype does not exist DROP OPERATOR IF EXISTS public.% (point, widget); ERROR: type widget does not exist DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); ERROR: type widget does not exist I'm not sure whether DROP CAST/OPERATOR/FUNCTION IF EXISTS should be more tolerant of nonexistent types, of if the mess could perhaps be avoided by dump reordering. Note, this usability problem affects unpatched head as well: pg_dump -Fc -d regression --file=regression.dump pg_restore --clean -1 -d regression regression.dump ... pg_restore: [archiver (db)] could not execute query: ERROR: type widget does not exist Command was: DROP FUNCTION public.widget_out(widget); (The use here is a little different than the first example above, but I would still expect this case to work.) The above problems with IF EXISTS aren't really a problem of the patch per se, but IMO it would be nice to straighten all the issues out together for 9.4. ok * -- additional check, available only with -c option Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) no some people - like we in our company would to use this feature for quiet and strict mode for plain text format too. enabling this feature has zero overhead so there are no reason block it anywhere. Some comments on the changes: 1. There is at least one IF EXISTS check missing from pg_dump.c, see for example this statement from a dump of the regression database with --if-exists: ALTER TABLE public.nv_child_2010 DROP CONSTRAINT nv_child_2010_d_check; 2. Shouldn't pg_restore get --if-exists as well? 3. + printf(_( --if-exists don't report error if cleaned object doesn't exist\n)); This help output bleeds just over our de facto 80-character limit. Also contractions seem to be avoided elsewhere. It's a little hard to squeeze a decent explanation into one line, but perhaps: Use IF EXISTS when dropping objects would be better. The sgml changes could use some wordsmithing and grammar fixes. I could clean these up for you in a later version if you'd like. 4. There seem to be spurious whitespace changes to the function prototype and declaration for _printTocEntry. I'll send updated version in next months Thank you for review Regards Pavel That's all I've had time for so far... Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/3/8 Josh Kupershmidt schmi...@gmail.com: Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) no some people - like we in our company would to use this feature for quiet and strict mode for plain text format too. enabling this feature has zero overhead so there are no reason block it anywhere. I'm not sure I understand what you're getting at, but maybe my proposal wasn't so clear. Right now, you can specify --clean along with -Fc to pg_dump, and pg_dump will not complain even though this combination is nonsense. I am proposing that pg_dump error out in this case, i.e. $ pg_dump -Fc --file=test.dump --clean -d test pg_dump: option --clean only valid with plain format dump Although this lack of an error a (IMO) misfeature of existing pg_dump, so if you'd rather leave this issue aside for your patch, that is fine. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
2013/3/8 Josh Kupershmidt schmi...@gmail.com: On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule pavel.steh...@gmail.com wrote: 2013/3/8 Josh Kupershmidt schmi...@gmail.com: Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) no some people - like we in our company would to use this feature for quiet and strict mode for plain text format too. enabling this feature has zero overhead so there are no reason block it anywhere. I'm not sure I understand what you're getting at, but maybe my proposal wasn't so clear. Right now, you can specify --clean along with -Fc to pg_dump, and pg_dump will not complain even though this combination is nonsense. I am proposing that pg_dump error out in this case, i.e. $ pg_dump -Fc --file=test.dump --clean -d test pg_dump: option --clean only valid with plain format dump Although this lack of an error a (IMO) misfeature of existing pg_dump, so if you'd rather leave this issue aside for your patch, that is fine. I'll see - please, stay tuned to 9.4 first commitfest Regards Pavel Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist
[Moving to -hackers] On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule pavel.steh...@gmail.com wrote: so * --conditional-drops replaced by --if-exists Thanks for the fixes, I played around with the patch a bit. I was sort of expecting this example to work (after setting up the regression database with `make installcheck`) pg_dump --clean --if-exists -Fp -d regression --file=regression.sql createdb test psql -v ON_ERROR_STOP=1 --single-transaction -d test -f regression.sql But it fails, first at: ... DROP TRIGGER IF EXISTS tsvectorupdate ON public.test_tsvector; ERROR: relation public.test_tsvector does not exist This seems like a shortcoming of DROP TRIGGER ... IF EXISTS, and it looks like DROP RULE ... IF EXISTS has the same problem. I recall DROP ... IF EXISTS being fixed recently for not to error out if the schema specified for the object does not exist, and ISTM the same arguments could be made in favor of fixing DROP TRIGGER/TABLE ... IF EXISTS not to error out if the table doesn't exist. Working further through the dump of the regression database, these also present problems for --clean --if-exists dumps: DROP CAST IF EXISTS (text AS public.casttesttype); ERROR: type public.casttesttype does not exist DROP OPERATOR IF EXISTS public.% (point, widget); ERROR: type widget does not exist DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget); ERROR: type widget does not exist I'm not sure whether DROP CAST/OPERATOR/FUNCTION IF EXISTS should be more tolerant of nonexistent types, of if the mess could perhaps be avoided by dump reordering. Note, this usability problem affects unpatched head as well: pg_dump -Fc -d regression --file=regression.dump pg_restore --clean -1 -d regression regression.dump ... pg_restore: [archiver (db)] could not execute query: ERROR: type widget does not exist Command was: DROP FUNCTION public.widget_out(widget); (The use here is a little different than the first example above, but I would still expect this case to work.) The above problems with IF EXISTS aren't really a problem of the patch per se, but IMO it would be nice to straighten all the issues out together for 9.4. * -- additional check, available only with -c option Cool. I think it would also be useful to check that --clean may only be used with --format=p to avoid any confusion there. (This issue could be addressed in a separate patch if you'd rather not lard this patch.) Some comments on the changes: 1. There is at least one IF EXISTS check missing from pg_dump.c, see for example this statement from a dump of the regression database with --if-exists: ALTER TABLE public.nv_child_2010 DROP CONSTRAINT nv_child_2010_d_check; 2. Shouldn't pg_restore get --if-exists as well? 3. + printf(_( --if-exists don't report error if cleaned object doesn't exist\n)); This help output bleeds just over our de facto 80-character limit. Also contractions seem to be avoided elsewhere. It's a little hard to squeeze a decent explanation into one line, but perhaps: Use IF EXISTS when dropping objects would be better. The sgml changes could use some wordsmithing and grammar fixes. I could clean these up for you in a later version if you'd like. 4. There seem to be spurious whitespace changes to the function prototype and declaration for _printTocEntry. That's all I've had time for so far... Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
