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

Sorry second update

fixed dump if-exists of DROP OPERATOR

Regards

Pavel

2014-01-25 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

> Hello
>
> I fixed all described issues. There was a more mistakes, that I fixed -
> main mistake was in work with te->desc variable.
>
> You propose a regress tests for pg_dump? I searched in mailing lists and
> there was some proposals about it. I am not against, but I have to do some
> research and better be this as separate patch.
>
> This patch requires b152c6cd0de1827ba58756e24e18110cf902182a commit.
>
> Regards
>
> Pavel
>
>
>
>
> 2014-01-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.
>>
>
>