Re: patch: ALTER TABLE IF EXISTS

Hello

this is relative simple patch that add possibility to skip noexisting
tables. It is necessary for silent cleaning when dump is loaded.

Regards

Pavel Stehule

On Mon, Jan 2, 2012 at 1:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> this is relative simple patch that add possibility to skip noexisting
> tables. It is necessary for silent cleaning when dump is loaded.

Agreed, nice simple and uncontentious patch.

All good apart from two minor things:

* doc page needs to explain what IF EXISTS does, like DROP TABLE, e.g.

IF EXISTS
Do not throw an error if the table does not exist. A notice is
issued in this case.

* the error message is "does not exist" rather than "does not exists",
which means the code, a comment and a test need changing

Other than that, happy to apply as soon as you make those changes.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2 Leden 2012, 14:11, Pavel Stehule wrote:
> Hello
>
> this is relative simple patch that add possibility to skip noexisting
> tables. It is necessary for silent cleaning when dump is loaded.

Just a curious question - what use case is solved by this? Under what
circumstances you get an ALTER TABLE without a preceding CREATE TABLE? I
can't think of such scenario ...

Or is this meant for scripts written manually so that it's possible to do
alter if the table already exists and create if it does not exist?

Tomas

Hello

2012/1/2 Tomas Vondra <tv(at)fuzzy(dot)cz>:
> On 2 Leden 2012, 14:11, Pavel Stehule wrote:
>> Hello
>>
>> this is relative simple patch that add possibility to skip noexisting
>> tables. It is necessary for silent cleaning when dump is loaded.
>
> Just a curious question - what use case is solved by this? Under what
> circumstances you get an ALTER TABLE without a preceding CREATE TABLE? I
> can't think of such scenario ...
>
> Or is this meant for scripts written manually so that it's possible to do
> alter if the table already exists and create if it does not exist?

this is necessary for "silent" cleaning in pg_dump

this is fragment of dump with -c option

ALTER TABLE ONLY public.b DROP CONSTRAINT b_b_fkey;
DROP INDEX public.a_a_idx;
ALTER TABLE ONLY public.a DROP CONSTRAINT a_pkey;
DROP TABLE public.b;
DROP TABLE public.a;
DROP EXTENSION plpgsql;
DROP SCHEMA public;

I am working on "silent cleaning" and I am able generate a sequence of
statements:

ALTER TABLE IF EXISTS ONLY public.b DROP CONSTRAINT b_b_fkey;
DROP INDEX IF EXISTS public.a_a_idx;
ALTER TABLE IF EXISTS ONLY public.a DROP CONSTRAINT a_pkey;
DROP TABLE IF EXISTS public.b;
DROP TABLE IF EXISTS public.a;
DROP EXTENSION IF EXISTS plpgsql;
DROP SCHEMA IF EXISTS public;

constraint b_b_fkey should be removed before dropping index a_a_idx

Now we have DROP .. IF EXISTS statements, but ALTER TABLE .. IF EXISTS missing

Regards

Pavel
>
> Tomas
>

Hello

here is updated patch

Regards

Pavel

2012/1/2 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> On Mon, Jan 2, 2012 at 1:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
>> this is relative simple patch that add possibility to skip noexisting
>> tables. It is necessary for silent cleaning when dump is loaded.
>
> Agreed, nice simple and uncontentious patch.
>
> All good apart from two minor things:
>
> * doc page needs to explain what IF EXISTS does, like DROP TABLE, e.g.
>
> IF EXISTS
>    Do not throw an error if the table does not exist. A notice is
> issued in this case.
>
> * the error message is "does not exist" rather than "does not exists",
> which means the code, a comment and a test need changing
>
> Other than that, happy to apply as soon as you make those changes.
>
> --
>  Simon Riggs                   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> here is updated patch

I think the comments in parse_utilcmd.c probably need a bit of adjustment.

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

Hello

2012/1/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> here is updated patch
>
> I think the comments in parse_utilcmd.c probably need a bit of adjustment.

I don't see it - there is only one comment and it is adjusted with
"if" statement.

please, show it

Regards

Pavel

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

On Tue, Jan 3, 2012 at 10:38 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> 2012/1/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> here is updated patch
>>
>> I think the comments in parse_utilcmd.c probably need a bit of adjustment.
>
> I don't see it - there is only one comment and it is adjusted with
> "if" statement.
>
> please, show it

Well, basically, the comment preceding the part you altered say "the
lock level requested here", but "here" is getting spread out quite a
bit more with this code change. Maybe that doesn't matter.

However, on further examination, this is a pretty awkward way to write
the code. Why not something like this:

rel = relation_openrv_extended(stmt->relation, lockmode, stmt->missing_ok);
if (rel == NULL)
{
ereport(...);
return NIL;
}

Maybe the intent of sticking heap_openrv_extended() into the upper
branch of the if statement is to try to bounce relations that aren't
tables, but that's not actually what that function does (e.g. a
foreign table will slip through). And I think if we're going to have
IF EXISTS support for ALTER TABLE at all, we ought to have it for the
whole family of related statements: ALTER VIEW, ALTER SEQUENCE, ALTER
FOREIGN TABLE, etc., not just ALTER TABLE itself.

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

Hello

2012/1/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jan 3, 2012 at 10:38 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Hello
>>
>> 2012/1/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Mon, Jan 2, 2012 at 12:01 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>> here is updated patch
>>>
>>> I think the comments in parse_utilcmd.c probably need a bit of adjustment.
>>
>> I don't see it - there is only one comment and it is adjusted with
>> "if" statement.
>>
>> please, show it
>
> Well, basically, the comment preceding the part you altered say "the
> lock level requested here", but "here" is getting spread out quite a
> bit more with this code change.  Maybe that doesn't matter.
>
> However, on further examination, this is a pretty awkward way to write
> the code.  Why not something like this:
>
> rel = relation_openrv_extended(stmt->relation, lockmode, stmt->missing_ok);
> if (rel == NULL)
> {
>    ereport(...);
>    return NIL;
> }
>
> Maybe the intent of sticking heap_openrv_extended() into the upper
> branch of the if statement is to try to bounce relations that aren't
> tables, but that's not actually what that function does (e.g. a
> foreign table will slip through).  And I think if we're going to have
> IF EXISTS support for ALTER TABLE at all, we ought to have it for the
> whole family of related statements: ALTER VIEW, ALTER SEQUENCE, ALTER
> FOREIGN TABLE, etc., not just ALTER TABLE itself.
>

jup, we can continue in enhancing step by step.

I change a patch and now ALTER TABLE, ALTER INDEX, ALTER SEQUENCE and
ALTER VIEW has IF EXISTS clause

Regards

Pavel

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

On Tue, Jan 3, 2012 at 7:49 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> I change a patch and now ALTER TABLE, ALTER INDEX, ALTER SEQUENCE and
> ALTER VIEW has IF EXISTS clause

Patch no longer applies. Pls update.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jan 3, 2012 at 2:49 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> jup, we can continue in enhancing step by step.
>
> I change a patch and now ALTER TABLE, ALTER INDEX, ALTER SEQUENCE and
> ALTER VIEW has IF EXISTS clause

ALTER FOREIGN TABLE should be parallel as well, I think.

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

Hello

2012/1/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jan 3, 2012 at 2:49 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> jup, we can continue in enhancing step by step.
>>
>> I change a patch and now ALTER TABLE, ALTER INDEX, ALTER SEQUENCE and
>> ALTER VIEW has IF EXISTS clause
>
> ALTER FOREIGN TABLE should be parallel as well, I think.
>

refreshed + ALTER FOREIGN TABLE IF EXISTS ... support

Regards

Pavel

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

On 23 January 2012 20:14, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> 2012/1/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Tue, Jan 3, 2012 at 2:49 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> jup, we can continue in enhancing step by step.
>>>
>>> I change a patch and now ALTER TABLE, ALTER INDEX, ALTER SEQUENCE and
>>> ALTER VIEW has IF EXISTS clause
>>
>> ALTER FOREIGN TABLE should be parallel as well, I think.
>>
>
> refreshed + ALTER FOREIGN TABLE IF EXISTS ... support
>

I just noticed this copy-and-paste error in the ALTER FOREIGN TABLE docs:

IF EXISTS:

Do not throw an error if the sequence does not exist. A notice is issued
in this case.

That should be "foreign table" not "sequence".

Regards,
Dean

On 27.01.2012 11:57, Dean Rasheed wrote:
> I just noticed this copy-and-paste error in the ALTER FOREIGN TABLE docs:
>
> IF EXISTS:
>
> Do not throw an error if the sequence does not exist. A notice is issued
> in this case.
>
> That should be "foreign table" not "sequence".

Thanks, fixed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com