Обсуждение: pg_dump quietly ignore missing tables - is it bug?

On Fri, Mar 13, 2015 at 11:26 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> we found possible bug in pg_dump. It raise a error only when all specified
> tables doesn't exists. When it find any table, then ignore missing other.
>
> /usr/local/pgsql/bin/pg_dump -t Foo -t omega -s postgres > /dev/null; echo
> $?
>
> foo doesn't exists - it creates broken backup due missing "Foo" table
>
>  [pavel@localhost include]$ /usr/local/pgsql/bin/pg_dump -t Foo -t omegaa -s
> postgres > /dev/null; echo $?
> pg_dump: No matching tables were found
> 1
>
> Is it ok? I am thinking, so it is potentially dangerous. Any explicitly
> specified table should to exists.

Keep in mind that the argument to -t is a pattern, not just a table
name.  I'm not sure how much that affects the calculus here, but it's
something to think about.

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

2015-03-13 17:39 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Fri, Mar 13, 2015 at 11:26 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> we found possible bug in pg_dump. It raise a error only when all specified
> tables doesn't exists. When it find any table, then ignore missing other.
>
> /usr/local/pgsql/bin/pg_dump -t Foo -t omega -s postgres > /dev/null; echo
> $?
>
> foo doesn't exists - it creates broken backup due missing "Foo" table
>
>  [pavel@localhost include]$ /usr/local/pgsql/bin/pg_dump -t Foo -t omegaa -s
> postgres > /dev/null; echo $?
> pg_dump: No matching tables were found
> 1
>
> Is it ok? I am thinking, so it is potentially dangerous. Any explicitly
> specified table should to exists.

Keep in mind that the argument to -t is a pattern, not just a table
name.  I'm not sure how much that affects the calculus here, but it's
something to think about.

yes, it has a sense, although now, I am don't think so it was a good idea. There should be some difference between table name and table pattern.

On 03/13/2015 10:01 AM, Pavel Stehule wrote:
>
>
> 2015-03-13 17:39 GMT+01:00 Robert Haas <robertmhaas@gmail.com
> <mailto:robertmhaas@gmail.com>>:
>
>     On Fri, Mar 13, 2015 at 11:26 AM, Pavel Stehule
>     <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> wrote:
>     > we found possible bug in pg_dump. It raise a error only when all
>     specified
>     > tables doesn't exists. When it find any table, then ignore missing
>     other.
>     >
>     > /usr/local/pgsql/bin/pg_dump -t Foo -t omega -s postgres >
>     /dev/null; echo
>     > $?
>     >
>     > foo doesn't exists - it creates broken backup due missing "Foo" table
>     >
>     >  [pavel@localhost include]$ /usr/local/pgsql/bin/pg_dump -t Foo -t
>     omegaa -s
>     > postgres > /dev/null; echo $?
>     > pg_dump: No matching tables were found
>     > 1
>     >
>     > Is it ok? I am thinking, so it is potentially dangerous. Any
>     explicitly
>     > specified table should to exists.
>
>     Keep in mind that the argument to -t is a pattern, not just a table
>     name.  I'm not sure how much that affects the calculus here, but it's
>     something to think about.
>
>
> yes, it has a sense, although now, I am don't think so it was a good
> idea. There should be some difference between table name and table pattern.

There was a long discussion about this when the feature was introduced
in 7.3(?) IIRC.  Changing it now would break backwards-compatibility, so
you'd really need to introduce a new option.

And, if you introduce a new option which is a specific table name, would
you require a schema name or not?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

2015-03-13 23:43 GMT+01:00 Josh Berkus <josh@agliodbs.com>:

On 03/13/2015 10:01 AM, Pavel Stehule wrote:
>
>
> 2015-03-13 17:39 GMT+01:00 Robert Haas <robertmhaas@gmail.com
> <mailto:robertmhaas@gmail.com>>:
>
>     On Fri, Mar 13, 2015 at 11:26 AM, Pavel Stehule
>     <pavel.stehule@gmail.com <mailto:pavel.stehule@gmail.com>> wrote:
>     > we found possible bug in pg_dump. It raise a error only when all
>     specified
>     > tables doesn't exists. When it find any table, then ignore missing
>     other.
>     >
>     > /usr/local/pgsql/bin/pg_dump -t Foo -t omega -s postgres >
>     /dev/null; echo
>     > $?
>     >
>     > foo doesn't exists - it creates broken backup due missing "Foo" table
>     >
>     >  [pavel@localhost include]$ /usr/local/pgsql/bin/pg_dump -t Foo -t
>     omegaa -s
>     > postgres > /dev/null; echo $?
>     > pg_dump: No matching tables were found
>     > 1
>     >
>     > Is it ok? I am thinking, so it is potentially dangerous. Any
>     explicitly
>     > specified table should to exists.
>
>     Keep in mind that the argument to -t is a pattern, not just a table
>     name.  I'm not sure how much that affects the calculus here, but it's
>     something to think about.
>
>
> yes, it has a sense, although now, I am don't think so it was a good
> idea. There should be some difference between table name and table pattern.

There was a long discussion about this when the feature was introduced
in 7.3(?) IIRC.  Changing it now would break backwards-compatibility, so
you'd really need to introduce a new option.

And, if you introduce a new option which is a specific table name, would
you require a schema name or not?

We can use a same rules like we use for STRICT clause somewhere. There should be only one table specified by the option. If it is not necessary, then only name is enough, else qualified name is necessary.

what is a name for this option? Maybe only with long name - some like 'required-table' ?

Regards

Pavel

 

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Pavel Stehule <pavel.stehule@gmail.com> writes:
> other variant, I hope better than previous. We can introduce new long
> option "--strict". With this active option, every pattern specified by -t
> option have to have identifies exactly only one table. It can be used for
> any other "should to exists" patterns - schemas. Initial implementation in
> attachment.

I think this design is seriously broken.  If I have '-t foo*' the code
should not prevent that from matching multiple tables.  What would the use
case for such a restriction be?

What would make sense to me is one or both of these ideas:

* require a match for a wildcard-free -t switch

* require at least one (not "exactly one") match for a wildcarded -t
  switch.

Neither of those is what you wrote, though.

If we implemented the second one of these, it would have to be controlled
by a new switch, because there are plausible use cases for wildcards that
sometimes don't match anything (not to mention backwards compatibility).
There might be a reasonable argument for the first one being the
default behavior, though; I'm not sure if we could get away with that
from a compatibility perspective.

                        regards, tom lane

Hi

2015-03-15 16:09 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:
> other variant, I hope better than previous. We can introduce new long
> option "--strict". With this active option, every pattern specified by -t
> option have to have identifies exactly only one table. It can be used for
> any other "should to exists" patterns - schemas. Initial implementation in
> attachment.

I think this design is seriously broken.  If I have '-t foo*' the code
should not prevent that from matching multiple tables.  What would the use
case for such a restriction be?

What would make sense to me is one or both of these ideas:

* require a match for a wildcard-free -t switch

* require at least one (not "exactly one") match for a wildcarded -t
  switch.

attached initial implementation

Regards

Pavel

 

Neither of those is what you wrote, though.

If we implemented the second one of these, it would have to be controlled
by a new switch, because there are plausible use cases for wildcards that
sometimes don't match anything (not to mention backwards compatibility).
There might be a reasonable argument for the first one being the
default behavior, though; I'm not sure if we could get away with that
from a compatibility perspective.

                        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
>
> 2015-03-23 17:11 GMT+01:00 Pavel Stehule <pavel.stehule@gmail.com>:
>
>> Hi
>>
>> 2015-03-15 16:09 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>
>>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>> > other variant, I hope better than previous. We can introduce new long
>>> > option "--strict". With this active option, every pattern specified by
>>> -t
>>> > option have to have identifies exactly only one table. It can be used
>>> for
>>> > any other "should to exists" patterns - schemas. Initial implementation
>>> in
>>> > attachment.
>>>
>>> I think this design is seriously broken.  If I have '-t foo*' the code
>>> should not prevent that from matching multiple tables.  What would the use
>>> case for such a restriction be?
>>>
>>> What would make sense to me is one or both of these ideas:
>>>
>>> * require a match for a wildcard-free -t switch
>>>
>>> * require at least one (not "exactly one") match for a wildcarded -t
>>>   switch.
>>>
>>
>>
>> attached initial implementation
>>
>
> updated version - same mechanism should be used for schema

Hello,

I think this is a bit over-engineered (apart from the fact that
processSQLNamePattern is also used in two dozen of places in
psql/describe.c and all of them must be touched for this patch to
compile).

Also, the new --table-if-exists options seems to be doing what the old
--table did, and I'm not really sure I underestand what --table does
now.

I propose instead to add a separate new option --strict-include, without
argument, that only controls the behavior when an include pattern didn't
find any table (or schema).

Please see attached patch.

--
Alex

2015-05-21 16:48 GMT+02:00 Oleksandr Shulgin <oleksandr.shulgin@zalando.de>:

I think this is a bit over-engineered (apart from the fact that
processSQLNamePattern is also used in two dozen of places in
psql/describe.c and all of them must be touched for this patch to
compile).

it was prototype - I believe so issue with describe.c can be solved better

 

Also, the new --table-if-exists options seems to be doing what the old
--table did, and I'm not really sure I underestand what --table does
now.

I propose instead to add a separate new option --strict-include, without
argument, that only controls the behavior when an include pattern didn't
find any table (or schema).

hard to say - any variant has own advantages and disadvantages

But I more to unlike it than like - it is more usual, when you use exact name so, you need it exactly one, and when you use some wildcard, so you are expecting one or more tables.

This use case is not checked in your patch.

On Fri, May 22, 2015 at 6:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-21 16:48 GMT+02:00 Oleksandr Shulgin <oleksandr.shulgin@zalando.de>:

I think this is a bit over-engineered (apart from the fact that
processSQLNamePattern is also used in two dozen of places in
psql/describe.c and all of them must be touched for this patch to
compile).

it was prototype - I believe so issue with describe.c can be solved better

 

Also, the new --table-if-exists options seems to be doing what the old
--table did, and I'm not really sure I underestand what --table does
now.

I propose instead to add a separate new option --strict-include, without
argument, that only controls the behavior when an include pattern didn't
find any table (or schema).

hard to say - any variant has own advantages and disadvantages

But I more to unlike it than like - it is more usual, when you use exact name so, you need it exactly one, and when you use some wildcard, so you are expecting one or more tables.

This use case is not checked in your patch.

Maybe I'm missing something, but I believe it's handled by

pg_dump -t mytables* --strict-include

so that it will error out if nothing was found for mytables* pattern.

--

Alex

2015-05-22 18:30 GMT+02:00 Shulgin, Oleksandr <oleksandr.shulgin@zalando.de>:

On Fri, May 22, 2015 at 6:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-21 16:48 GMT+02:00 Oleksandr Shulgin <oleksandr.shulgin@zalando.de>:

I think this is a bit over-engineered (apart from the fact that
processSQLNamePattern is also used in two dozen of places in
psql/describe.c and all of them must be touched for this patch to
compile).

it was prototype - I believe so issue with describe.c can be solved better

 

Also, the new --table-if-exists options seems to be doing what the old
--table did, and I'm not really sure I underestand what --table does
now.

I propose instead to add a separate new option --strict-include, without
argument, that only controls the behavior when an include pattern didn't
find any table (or schema).

hard to say - any variant has own advantages and disadvantages

But I more to unlike it than like - it is more usual, when you use exact name so, you need it exactly one, and when you use some wildcard, so you are expecting one or more tables.

This use case is not checked in your patch.

Maybe I'm missing something, but I believe it's handled by

pg_dump -t mytables* --strict-include

so that it will error out if nothing was found for mytables* pattern.

If I understand it raise a error when it found more than one table

On Fri, May 22, 2015 at 6:32 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-22 18:30 GMT+02:00 Shulgin, Oleksandr <oleksandr.shulgin@zalando.de>:

On Fri, May 22, 2015 at 6:09 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-21 16:48 GMT+02:00 Oleksandr Shulgin <oleksandr.shulgin@zalando.de>:

I think this is a bit over-engineered (apart from the fact that
processSQLNamePattern is also used in two dozen of places in
psql/describe.c and all of them must be touched for this patch to
compile).

it was prototype - I believe so issue with describe.c can be solved better

 

Also, the new --table-if-exists options seems to be doing what the old
--table did, and I'm not really sure I underestand what --table does
now.

I propose instead to add a separate new option --strict-include, without
argument, that only controls the behavior when an include pattern didn't
find any table (or schema).

hard to say - any variant has own advantages and disadvantages

But I more to unlike it than like - it is more usual, when you use exact name so, you need it exactly one, and when you use some wildcard, so you are expecting one or more tables.

This use case is not checked in your patch.

Maybe I'm missing something, but I believe it's handled by

pg_dump -t mytables* --strict-include

so that it will error out if nothing was found for mytables* pattern.

If I understand it raise a error when it found more than one table

I hope not, and that totally was not my intent :-p

It should raise if it found *less than* one, that is: none.

Oleksandr Shulgin <oleksandr.shulgin@zalando.de> writes:
> I think this is a bit over-engineered (apart from the fact that
> processSQLNamePattern is also used in two dozen of places in
> psql/describe.c and all of them must be touched for this patch to
> compile).

> Also, the new --table-if-exists options seems to be doing what the old
> --table did, and I'm not really sure I underestand what --table does
> now.

I'm pretty sure we had agreed *not* to change the default behavior of -t.

> I propose instead to add a separate new option --strict-include, without
> argument, that only controls the behavior when an include pattern didn't
> find any table (or schema).

If we do it as a separate option, then it necessarily changes the behavior
for *each* -t switch in the call.  Can anyone show a common use-case where
that's no good, and you need separate behavior for each of several -t
switches?  If not, I like the simplicity of this approach.  (Perhaps the
switch name could use some bikeshedding, though.)

                        regards, tom lane

Oleksandr Shulgin <oleksandr.shulgin@zalando.de> writes:
> I think this is a bit over-engineered (apart from the fact that
> processSQLNamePattern is also used in two dozen of places in
> psql/describe.c and all of them must be touched for this patch to
> compile).

> Also, the new --table-if-exists options seems to be doing what the old
> --table did, and I'm not really sure I underestand what --table does
> now.

I'm pretty sure we had agreed *not* to change the default behavior of -t.

On Sat, May 23, 2015 at 1:41 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-05-22 18:34 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>
>> Oleksandr Shulgin <oleksandr.shulgin@zalando.de> writes:
>> > I think this is a bit over-engineered (apart from the fact that
>> > processSQLNamePattern is also used in two dozen of places in
>> > psql/describe.c and all of them must be touched for this patch to
>> > compile).
>>
>> > Also, the new --table-if-exists options seems to be doing what the old
>> > --table did, and I'm not really sure I underestand what --table does
>> > now.
>>
>> I'm pretty sure we had agreed *not* to change the default behavior of -t.
>>
>> > I propose instead to add a separate new option --strict-include, without
>> > argument, that only controls the behavior when an include pattern didn't
>> > find any table (or schema).
>>
>> If we do it as a separate option, then it necessarily changes the behavior
>> for *each* -t switch in the call.  Can anyone show a common use-case where
>> that's no good, and you need separate behavior for each of several -t
>> switches?  If not, I like the simplicity of this approach.  (Perhaps the
>> switch name could use some bikeshedding, though.)
>
>
> it is near to one proposal
>
> implement only new long option "--required-table"

There is no updated version of the patch. So I marked this patch
as "Waiting on Author".

One very simple question is, doesn't -n option have very similar problem?
Also what about -N, -T and --exclude-table-data? Not sure if we need to
handle them in the similar way as you proposed.

Isn't it sufficient to only emit the warning message to stderr if there
is no table matching the pattern specified in -t?

Regards,

--
Fujii Masao

Hi

I am sending updated version. It implements new long option "--strict-names". If this option is used, then for any entered name (without any wildcard char) must be found least one object. This option has not impact on patters (has wildcards chars). When this option is not used, then behave is 100% same as before (with same numbers of SQL queries for -t option). It is based on Oleksandr's documentation (and lightly modified philosophy), and cleaned my previous patch. A test on wildchard existency "strcspn(cell->val, "?*")" cannot be used, because it doesn't calculate quotes (but a replace has few lines only).

There is a possibility to remove a wildcard char test and require least one entry for patters too. But I am thinking, when somebody explicitly uses any wildcard, then he calculate with a possibility of empty result.

Regards

Pavel

2015-07-09 22:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-07-08 5:36 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Sat, May 23, 2015 at 1:41 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-05-22 18:34 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>
>> Oleksandr Shulgin <oleksandr.shulgin@zalando.de> writes:
>> > I think this is a bit over-engineered (apart from the fact that
>> > processSQLNamePattern is also used in two dozen of places in
>> > psql/describe.c and all of them must be touched for this patch to
>> > compile).
>>
>> > Also, the new --table-if-exists options seems to be doing what the old
>> > --table did, and I'm not really sure I underestand what --table does
>> > now.
>>
>> I'm pretty sure we had agreed *not* to change the default behavior of -t.
>>
>> > I propose instead to add a separate new option --strict-include, without
>> > argument, that only controls the behavior when an include pattern didn't
>> > find any table (or schema).
>>
>> If we do it as a separate option, then it necessarily changes the behavior
>> for *each* -t switch in the call.  Can anyone show a common use-case where
>> that's no good, and you need separate behavior for each of several -t
>> switches?  If not, I like the simplicity of this approach.  (Perhaps the
>> switch name could use some bikeshedding, though.)
>
>
> it is near to one proposal
>
> implement only new long option "--required-table"

There is no updated version of the patch. So I marked this patch
as "Waiting on Author".

tomorrow I'll return to this topic.

 

One very simple question is, doesn't -n option have very similar problem?
Also what about -N, -T and --exclude-table-data? Not sure if we need to
handle them in the similar way as you proposed.

hard to say - I understand to your motivation, and it can signalize some inconsistency in environment, but it has not same negative impact as same problem with -t -n options.

This is maybe place for warning message with possibility to disable this warning. But maybe it is premature optimization?

Next way is introduction of "strictness" option - default can be equivalent of current, safe can check only tables required for dump, strict can check all.

 

Isn't it sufficient to only emit the warning message to stderr if there
is no table matching the pattern specified in -t?

I prefer raising error in this case.

1. I am thinking so missing tables for dump signalize important inconsistency in environment and it is important bug

2. The warning is not simply detected (and processed) in bash scripts.

Regards

Pavel

 

Regards,

--
Fujii Masao

2015-07-19 20:54 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

I am sending updated version. It implements new long option "--strict-names". If this option is used, then for any entered name (without any wildcard char) must be found least one object. This option has not impact on patters (has wildcards chars). When this option is not used, then behave is 100% same as before (with same numbers of SQL queries for -t option). It is based on Oleksandr's documentation (and lightly modified philosophy), and cleaned my previous patch. A test on wildchard existency "strcspn(cell->val, "?*")" cannot be used, because it doesn't calculate quotes (but a replace has few lines only).

There is a possibility to remove a wildcard char test and require least one entry for patters too. But I am thinking, when somebody explicitly uses any wildcard, then he calculate with a possibility of empty result.

other variant is using --strict-names behave as default (and implement negative option like --disable-strict-names or some similar).

 

Regards

Pavel

2015-07-09 22:48 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2015-07-08 5:36 GMT+02:00 Fujii Masao <masao.fujii@gmail.com>:

On Sat, May 23, 2015 at 1:41 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-05-22 18:34 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>>
>> Oleksandr Shulgin <oleksandr.shulgin@zalando.de> writes:
>> > I think this is a bit over-engineered (apart from the fact that
>> > processSQLNamePattern is also used in two dozen of places in
>> > psql/describe.c and all of them must be touched for this patch to
>> > compile).
>>
>> > Also, the new --table-if-exists options seems to be doing what the old
>> > --table did, and I'm not really sure I underestand what --table does
>> > now.
>>
>> I'm pretty sure we had agreed *not* to change the default behavior of -t.
>>
>> > I propose instead to add a separate new option --strict-include, without
>> > argument, that only controls the behavior when an include pattern didn't
>> > find any table (or schema).
>>
>> If we do it as a separate option, then it necessarily changes the behavior
>> for *each* -t switch in the call.  Can anyone show a common use-case where
>> that's no good, and you need separate behavior for each of several -t
>> switches?  If not, I like the simplicity of this approach.  (Perhaps the
>> switch name could use some bikeshedding, though.)
>
>
> it is near to one proposal
>
> implement only new long option "--required-table"

There is no updated version of the patch. So I marked this patch
as "Waiting on Author".

tomorrow I'll return to this topic.

 

One very simple question is, doesn't -n option have very similar problem?
Also what about -N, -T and --exclude-table-data? Not sure if we need to
handle them in the similar way as you proposed.

hard to say - I understand to your motivation, and it can signalize some inconsistency in environment, but it has not same negative impact as same problem with -t -n options.

This is maybe place for warning message with possibility to disable this warning. But maybe it is premature optimization?

Next way is introduction of "strictness" option - default can be equivalent of current, safe can check only tables required for dump, strict can check all.

 

Isn't it sufficient to only emit the warning message to stderr if there
is no table matching the pattern specified in -t?

I prefer raising error in this case.

1. I am thinking so missing tables for dump signalize important inconsistency in environment and it is important bug

2. The warning is not simply detected (and processed) in bash scripts.

Regards

Pavel

 

Regards,

--
Fujii Masao

On 07/25/2015 07:08 PM, Pavel Stehule wrote:

I am sending a new patch - without checking wildcard chars.

The documentation says the option is called --strict-names, while the code has --strict-mode. I like --strict-names more, "mode" seems redundant, and it's not clear what it's strict about.

For symmetry, it would be good to also support this option in pg_restore. It seems even more useful there.

Can we do better than issuing a separate query for each table/schema name? The performance of this isn't very important, but still it seems like you could fairly easily refactor the code to avoid that. Perhaps return an extra constant for part of the UNION to distinguish which result row came from which pattern, and check that at least one row is returned for each.

- Heikki