Re: patch: option --if-exists for pg_dump

Tomorrow I'll send updated version

Regards

Pavel

2014/1/21 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>

> Hi Pavel,
>
> Consider following test scenario:
>
> mydb=# \d emp
> Table "public.emp"
> Column | Type | Modifiers
> --------+---------+-----------
> empno | integer | not null
> deptno | integer |
> ename | text |
> Indexes:
> "emp_pkey" PRIMARY KEY, btree (empno)
> Foreign-key constraints:
> "emp_deptno_fkey" FOREIGN KEY (deptno) REFERENCES dept(deptno)
>
> mydb=# \d dept
> Table "public.dept"
> Column | Type | Modifiers
> --------+---------+-----------
> deptno | integer | not null
> dname | text |
> Indexes:
> "dept_pkey" PRIMARY KEY, btree (deptno)
> Referenced by:
> TABLE "emp" CONSTRAINT "emp_deptno_fkey" FOREIGN KEY (deptno)
> REFERENCES dept(deptno)
>
> mydb=# \q
> jeevan(at)ubuntu:~/pg_master$ ./install/bin/pg_dump -d mydb --if-exists -c >
> mydb_ic.dmp
>
> I see following lines in dump which looks certainly wrong:
> ===
>
> DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_deptno_fkey;
> DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_pkey;
> DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept_pkey;
>
> When try to restore, as expected it is throwing an error:
> ===
>
> psql:mydb_ic.dmp:14: ERROR: syntax error at or near "FK"
> LINE 1: DROP FK CONSTRAINT IF EXISTS ublic.emp DROP CONSTRAINT emp_d...
> ^
> psql:mydb_ic.dmp:15: ERROR: syntax error at or near "CONSTRAINT"
> LINE 1: DROP CONSTRAINT IF EXISTS Y public.emp DROP CONSTRAINT emp_p...
> ^
> psql:mydb_ic.dmp:16: ERROR: syntax error at or near "CONSTRAINT"
> LINE 1: DROP CONSTRAINT IF EXISTS Y public.dept DROP CONSTRAINT dept...
> ^
>
> Note:
> ===
> Commands which are in form of ALTER TABLE ... DROP are failing.
> You need to test each and every object with DROP .. IF EXISTS command.
> Better write small test-case with all objects included.
>
> Following logic has flaw:
> ===
> diff --git a/src/bin/pg_dump/pg_backup_archiver.c
> b/src/bin/pg_dump/pg_backup_archiver.c
> index 7fc0288..0677712 100644
> --- a/src/bin/pg_dump/pg_backup_archiver.c
> +++ b/src/bin/pg_dump/pg_backup_archiver.c
> @@ -413,8 +413,30 @@ RestoreArchive(Archive *AHX)
> /* Select owner and schema as necessary */
> _becomeOwner(AH, te);
> _selectOutputSchema(AH, te->namespace);
> - /* Drop it */
> - ahprintf(AH, "%s", te->dropStmt);
> +
> + if (*te->dropStmt != '\0')
> + {
> + /* Inject IF EXISTS clause when it is required. */
> + if (ropt->if_exists)
> + {
> + char buffer[40];
> + size_t l;
> +
> + /* But do it only, when it is not there yet. */
> + snprintf(buffer, sizeof(buffer), "DROP %s IF
> EXISTS",
> + te->desc);
> + l = strlen(buffer);
> +
> + if (strncmp(te->dropStmt, buffer, l) != 0)
> + {
>
> + ahprintf(AH, "DROP %s IF EXISTS %s",
> + te->desc,
> + te->dropStmt + l);
> + }
> + else
> + ahprintf(AH, "%s", te->dropStmt);
> + }
> + }
> }
> }
>
>
> Also:
> ===
>
> 1.
> This is still out of sync.
>
> @@ -348,6 +350,8 @@ main(int argc, char *argv[])
> appendPQExpBufferStr(pgdumpopts, " --binary-upgrade");
> if (column_inserts)
> appendPQExpBufferStr(pgdumpopts, " --column-inserts");
> + if (if_exists)
> + appendPQExpBufferStr(pgdumpopts, " --if-exists");
> if (disable_dollar_quoting)
> appendPQExpBufferStr(pgdumpopts, " --disable-dollar-quoting");
> if (disable_triggers)
>
> 2.
> Spell check required:
>
> + /* skip first n chars, and create a modifieble copy */
>
> modifieble => modifiable
>
> + /* DROP IF EXISTS pattern is not appliable on dropStmt */
>
> appliable => applicable
>
> 3.
>
> + /*
> + * Object description is based on dropStmt statement. But
> + * a drop statements can be enhanced about IF EXISTS clause.
> + * We have to increase a offset in this case, "IF EXISTS"
> + * should not be included on object description.
> + */
>
> Looks like you need to re-phrase these comments line. Something like:
>
> /*
> * Object description is based on dropStmt statement which may have
> * IF EXISTS clause. Thus we need to update an offset such that it
> * won't be included in the object description.
> */
> Or as per your choice.
>
>
> Need to have careful thought on a bug mentioned above.
>
> Thanks
>
>
> On Fri, Jan 17, 2014 at 6:23 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:
>
>> Hello
>>
>>
>> 2014/1/16 Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>
>>
>>> Hi Pavel,
>>>
>>> I have reviewed the patch and here are my concerns and notes:
>>>
>>> POSITIVES:
>>> ---
>>> 1. Patch applies with some white-space errors.
>>> 2. make / make install / make check is smooth. No issues as such.
>>> 3. Feature looks good as well.
>>> 4. NO concern on overall design.
>>> 5. Good work.
>>>
>>>
>>> NEGATIVES:
>>> ---
>>>
>>> Here are the points which I see in the review and would like you to have
>>> your attention.
>>>
>>> 1.
>>> + It use conditional commands (with <literal>IF EXISTS</literal>
>>>
>>> Grammar mistakes. use => uses
>>>
>>> 2.
>>> @@ -55,7 +55,8 @@ static ArchiveHandle *_allocAH(const char *FileSpec,
>>> const ArchiveFormat fmt,
>>> const int compression, ArchiveMode mode, SetupWorkerPtr
>>> setupWorkerPtr);
>>> static void _getObjectDescription(PQExpBuffer buf, TocEntry *te,
>>> ArchiveHandle *AH);
>>> -static void _printTocEntry(ArchiveHandle *AH, TocEntry *te,
>>> RestoreOptions *ropt, bool isData, bool acl_pass);
>>> +static void _printTocEntry(ArchiveHandle *AH, TocEntry *te,
>>> RestoreOptions *ropt,
>>> + bool isData, bool acl_pass);
>>> static char *replace_line_endings(const char *str);
>>> static void _doSetFixedOutputState(ArchiveHandle *AH);
>>> static void _doSetSessionAuth(ArchiveHandle *AH, const char *user);
>>> @@ -234,6 +235,7 @@ RestoreArchive(Archive *AHX)
>>> bool parallel_mode;
>>> TocEntry *te;
>>> OutputContext sav;
>>> +
>>>
>>> AH->stage = STAGE_INITIALIZING;
>>>
>>> @@ -2961,7 +3005,8 @@ _getObjectDescription(PQExpBuffer buf, TocEntry
>>> *te, ArchiveHandle *AH)
>>> }
>>>
>>> static void
>>> -_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt,
>>> bool isData, bool acl_pass)
>>> +_printTocEntry(ArchiveHandle *AH, TocEntry *te, RestoreOptions *ropt,
>>> bool isData,
>>> + bool acl_pass)
>>> {
>>> /* ACLs are dumped only during acl pass */
>>> if (acl_pass)
>>>
>>> Above changes are NOT at all related to the patch. Please remove them
>>> even
>>> though they are clean-up like changes. Don't mix them with actual
>>> changes.
>>>
>>> 3.
>>> + if (strncmp(te->dropStmt + desc_len + 5, " IF
>>> EXISTS", 9) != 0)
>>>
>>> " IF EXISTS" has 10 characters NOT 9.
>>>
>>> 4.
>>> + if (strncmp(te->dropStmt + desc_len + 5, " IF
>>> EXISTS", 9) != 0)
>>> + ahprintf(AH, "DROP %s IF EXISTS %s",
>>> + te->desc,
>>> + te->dropStmt + 6 + desc_len);
>>>
>>> Here you have used strncmp, starting at te->dropStmt + X,
>>> where X = desc_len + 5. While adding back you used X = 6 + desc_len.
>>> First time you used 5 as you added space in comparison but for adding
>>> back we
>>> want past space location and thus you have used 6. That's correct, but
>>> little
>>> bit confusing. Why not you simply used
>>> + if (strstr(te->dropStmt, "IF EXISTS") != NULL)
>>> to check whether drop statement has "IF EXISTS" or not like you did at
>>> some
>>> other place. This will remove my concern 3 and above confusion as well.
>>> What you think ?
>>>
>>> 5.
>>> + }
>>> +
>>> + else
>>>
>>> Extra line before else part. Please remove it for consistency.
>>>
>>> 6.
>>> + printf(_(" --if-exists use IF EXISTS when dropping
>>> objects\n")); (pg_dump)
>>> + printf(_(" --if-exists don't report error if
>>> cleaned object doesn't exist\n")); (pg_dumpall)
>>> + printf(_(" --if-exists use IF EXISTS when dropping
>>> objects\n")); (pg_restore)
>>>
>>> Please have same message for all three.
>>>
>>> 7.
>>> printf(_(" --binary-upgrade for use by upgrade
>>> utilities only\n"));
>>> printf(_(" --column-inserts dump data as INSERT
>>> commands with column names\n"));
>>> + printf(_(" --if-exists don't report error if
>>> cleaned object doesn't exist\n"));
>>> printf(_(" --disable-dollar-quoting disable dollar quoting, use
>>> SQL standard quoting\n"));
>>> printf(_(" --disable-triggers disable triggers during
>>> data-only restore\n"));
>>>
>>> Please maintain order like pg_dump and pg_restore. Also at variable
>>> declaration
>>> and at options parsing mechanism.
>>>
>>>
>> I fixed a previous issue, see a attachment please
>>
>>
>>> 8.
>>> + if (if_exists && !outputClean)
>>> + exit_horribly(NULL, "option --if-exists requires -c/--clean
>>> option\n");
>>>
>>> Are we really want to exit when -c is not provided ? Can't we simply
>>> ignore
>>> --if-exists in that case (or with emitting a warning) ?
>>>
>>>
>> This behave is based on a talk related to proposal of this feature - and
>> I am thinking, this behave is little bit safer - ignoring requested
>> functionality is not what I like. And a error message is simple and clean
>> in this case - is not difficult to use it and it is not difficult to fix
>> missing option for user
>>
>> Regards
>>
>> Pavel
>>
>>
>>
>>
>>> Marking "Waiting on author".
>>>
>>> Thanks
>>>
>>>
>>> --
>>> Jeevan B Chalke
>>> Principal Software Engineer, Product Development
>>> EnterpriseDB Corporation
>>> The Enterprise PostgreSQL Company
>>>
>>>
>>
>
>
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 30589500
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>