Обсуждение: 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.stehule@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

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
> [Moving to -hackers]
>
> On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule <pavel.stehule@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

On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2013/3/8 Josh Kupershmidt <schmiddy@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

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
> On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2013/3/8 Josh Kupershmidt <schmiddy@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

On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@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

2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:
> On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@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

Hello

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
> On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> 2013/3/8 Josh Kupershmidt <schmiddy@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

Hello

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
> [Moving to -hackers]
>
> On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule <pavel.stehule@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

Hello

remastered patch

still there is a issue with dependencies

Regards

Pavel Stehule




2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:
> On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@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

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.stehule@gmail.com>:
> Hello
>
> remastered patch
>
> still there is a issue with dependencies
>
> Regards
>
> Pavel Stehule
>
>
>
>
> 2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:
>> On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@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

On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule <pavel.stehule@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

<p dir="ltr">Yes, I wrote a separate patch for next commitfest.<div class="gmail_quote">Dne 10.7.2013 16:54 "Josh
Kupershmidt"<<a href="mailto:schmiddy@gmail.com">schmiddy@gmail.com</a>> napsal(a):<br type="attribution"
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Fri, Jul 5,
2013at 12:16 PM, Pavel Stehule <<a href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>> wrote:<br
/><br/> > I am sending a patch that removes strict requirements for DROP IF<br /> > EXISTS statements. This
behaveis similar to our ALTER IF EXISTS<br /> > behave now.<br /><br /> +1 for this idea. But this patch should be
treatedas a separate issue<br /> from the use of IF EXISTS in pg_dump/pg_restore, right? If so, I<br /> suggest
startinga new thread about this patch to make reviewing<br /> easier.<br /><br /> Josh<br /></blockquote></div> 

On Tue, Jul 2, 2013 at 5:39 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

> 2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
>> On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>>> 2013/3/8 Josh Kupershmidt <schmiddy@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

On Tue, Jul 2, 2013 at 7:47 AM, Pavel Stehule <pavel.stehule@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-JSzA@mail.gmail.com

(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 <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp

On 09/19/2013 06:12 PM, Pavel Stehule wrote:
>
>
> 2013/9/16 Satoshi Nagayasu <snaga@uptime.jp <mailto:snaga@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

On Thu, Oct 10, 2013 at 12:54 PM, Andrew Dunstan <andrew@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

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 <snaga@uptime.jp <mailto:snaga@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

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 <snaga@uptime.jp <mailto:snaga@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

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 <snaga@uptime.jp <mailto:snaga@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

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 <snaga@uptime.jp
>>> <mailto:snaga@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

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 <snaga@uptime.jp
> >>> <mailto:snaga@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

On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt <schmiddy@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=vQ@mail.gmail.com

On Tue, Oct 22, 2013 at 9:37 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> On Mon, Oct 14, 2013 at 5:38 PM, Josh Kupershmidt <schmiddy@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

[ catching up on old email ]

Andres Freund <andres@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

On 2013-11-10 16:28:27 -0500, Tom Lane wrote:
> [ catching up on old email ]
> 
> Andres Freund <andres@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

Andres Freund <andres@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

On 2013-11-10 18:16:16 -0500, Tom Lane wrote:
> Andres Freund <andres@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

Andres Freund <andres@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

On 2013-11-10 18:26:26 -0500, Tom Lane wrote:
> Andres Freund <andres@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

Andres Freund <andres@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

On 2013-11-10 18:42:11 -0500, Tom Lane wrote:
> Andres Freund <andres@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

Andres Freund <andres@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

On 12 November 2013 16:00, Pavel Stehule <pavel.stehule@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

On Tue, Nov 19, 2013 at 3:53 AM, Dean Rasheed <dean.a.rasheed@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

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.

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

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.

On 26 November 2013 19:54, Peter Eisentraut <peter_e@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

Dean Rasheed <dean.a.rasheed@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

<div dir="ltr">I'll prepare patch<br /></div><div class="gmail_extra"><br /><br /><div class="gmail_quote">2013/11/27
TomLane <span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span><br
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="im">DeanRasheed <<a href="mailto:dean.a.rasheed@gmail.com">dean.a.rasheed@gmail.com</a>> writes:<br />
>Actually the IF EXISTS in DROP TABLE now applies to the schema as<br /> > well. Unfortunately there is currently
noconsistency across the<br /> > various DROP commands --- some tolerate a non-existent schema, while<br /> >
otherserror out.<br /><br /></div>Yeah.  I think now that we've had this discussion, we should make them<br /> all
toleratea non-existent schema.  I'm fine with having that happen<br /> over a series of patches rather than all at once
though.<br/><div class="im"><br /> > Also amongst those that tolerate a non-existent<br /> > schema, the
resultingnotices are not consistent --- some report the<br /> > schema-qualified object name, while others just
reportthe local<br /> > object name.<br /><br /></div>Less excited about this part, but on the whole I'd vote for
the"schema<br /> "no_such_schema" does not exist" wording in cases where the schema isn't<br /> there.  The other way
isless specific for no very good reason.<br /><br />                         regards, tom lane<br
/></blockquote></div><br/></div> 

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.

On 1 December 2013 07:32, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> 2013/11/30 Peter Eisentraut <peter_e@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2 December 2013 19:37, Alvaro Herrera <alvherre@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On 2 December 2013 04:55, Pavel Stehule <pavel.stehule@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

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.

On 5 December 2013 01:33, Peter Eisentraut <peter_e@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

On 5 December 2013 10:06, Pavel Stehule <pavel.stehule@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

On 7 December 2013 21:34, Pavel Stehule <pavel.stehule@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.

Nice work!

Regards,
Dean

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 21 January 2014 22:28, Alvaro Herrera <alvherre@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services