Thread: ALTER TABLESPACE MOVE command tag tweak

ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and
as such, I think event triggers should support that command.  I'm not
proposing to change event triggers at this stage, but since IMO we will
want to do that in 9.5, we need it to have a different command tag than
plain ALTER TABLESPACE.  This is so that check_ddl_tag() can compare
the tag with ALTER TABLESPACE and say "unsupported", and ALTER
TABLESPACE MOVE and say "supported".  Both are currently spelled the
same, which will be a problem.

Therefore I propose the attached patch for 9.4.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: ALTER TABLESPACE MOVE command tag tweak

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and
> as such, I think event triggers should support that command.  I'm not
> proposing to change event triggers at this stage, but since IMO we will
> want to do that in 9.5, we need it to have a different command tag than
> plain ALTER TABLESPACE.  This is so that check_ddl_tag() can compare
> the tag with ALTER TABLESPACE and say "unsupported", and ALTER
> TABLESPACE MOVE and say "supported".  Both are currently spelled the
> same, which will be a problem.

> Therefore I propose the attached patch for 9.4.

Hm.  While the specific change here seems harmless enough, the argument
for it seems to me to indicate that the very design is broken.  Do you
expect event triggers to distinguish all the different subflavors of
ALTER TABLE, for example, on the basis of the command tag?  Backwards
compatibility is going to prevent us from refining the tag strings
that much.  So ISTM this concern means what we'd better be thinking
about is some other way for event triggers to find out what they're
dealing with.

A different thought is that what event triggers would probably like
is for this command to be reported to them as a series of
ALTER TABLE SET TABLESPACE events, one per moved table.  If it's
done like that then the tag for the outer ALTER TABLESPACE may not
be so important.
        regards, tom lane



Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > The ALTER TABLESPACE MOVE command affects tables, not tablespaces; and
> > as such, I think event triggers should support that command.  I'm not
> > proposing to change event triggers at this stage, but since IMO we will
> > want to do that in 9.5, we need it to have a different command tag than
> > plain ALTER TABLESPACE.  This is so that check_ddl_tag() can compare
> > the tag with ALTER TABLESPACE and say "unsupported", and ALTER
> > TABLESPACE MOVE and say "supported".  Both are currently spelled the
> > same, which will be a problem.
> 
> > Therefore I propose the attached patch for 9.4.
> 
> Hm.  While the specific change here seems harmless enough, the argument
> for it seems to me to indicate that the very design is broken.  Do you
> expect event triggers to distinguish all the different subflavors of
> ALTER TABLE, for example, on the basis of the command tag?  Backwards
> compatibility is going to prevent us from refining the tag strings
> that much.

Actually, I don't -- I have already implemented ALTER TABLE for event
triggers, and there wasn't any need to tweak the command tags there.
The problem in this particular case is that TABLESPACE is a "global"
object, thus not supported; but ALTER TABLESPACE MOVE is a command that
modifies tables (which *are* supported), not tablespaces.

ALTER TABLESPACE MOVE is a glorified ALTER TABLE.  If ALTER TABLESPACE
MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not
ALTER TABLESPACE.  Individually, since the implementation works by
calling AlterTableInternal(), it already works.

Now if you state that the current design in event_triggers that works by
slicing CommandTag and comparing the pieces is broken, I don't disagree
and I think I have now (in the patch posted in a nearby thread) some
more infrastructure to do it differently.  But even if we do that, I
think we're going to need a way to differentiate ALTER TABLESPACE MOVE
from other forms of ALTER TABLESPACE.  I haven't given this much
thought, though.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> The problem in this particular case is that TABLESPACE is a "global"
> object, thus not supported; but ALTER TABLESPACE MOVE is a command that
> modifies tables (which *are* supported), not tablespaces.

> ALTER TABLESPACE MOVE is a glorified ALTER TABLE.  If ALTER TABLESPACE
> MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not
> ALTER TABLESPACE.  Individually, since the implementation works by
> calling AlterTableInternal(), it already works.

> Now if you state that the current design in event_triggers that works by
> slicing CommandTag and comparing the pieces is broken, I don't disagree
> and I think I have now (in the patch posted in a nearby thread) some
> more infrastructure to do it differently.  But even if we do that, I
> think we're going to need a way to differentiate ALTER TABLESPACE MOVE
> from other forms of ALTER TABLESPACE.  I haven't given this much
> thought, though.

Yeah, I'd not paid much attention to it either.  Now that I look at it,
ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming
all around, because (unless I'm misunderstanding) it doesn't actually
alter any property of the tablespace itself.  It might be a bit late
to propose this, but I wonder if some syntax along the lines of
  MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar

wouldn't be less confusing.  Not sure what we'd use as command tag
for it though (not MOVE, since that's taken).
        regards, tom lane



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > ALTER TABLESPACE MOVE is a glorified ALTER TABLE.  If ALTER TABLESPACE
> > MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not
> > ALTER TABLESPACE.  Individually, since the implementation works by
> > calling AlterTableInternal(), it already works.
>
> > Now if you state that the current design in event_triggers that works by
> > slicing CommandTag and comparing the pieces is broken, I don't disagree
> > and I think I have now (in the patch posted in a nearby thread) some
> > more infrastructure to do it differently.  But even if we do that, I
> > think we're going to need a way to differentiate ALTER TABLESPACE MOVE
> > from other forms of ALTER TABLESPACE.  I haven't given this much
> > thought, though.
>
> Yeah, I'd not paid much attention to it either.  Now that I look at it,
> ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming
> all around, because (unless I'm misunderstanding) it doesn't actually
> alter any property of the tablespace itself.  It might be a bit late
> to propose this, but I wonder if some syntax along the lines of

I'm not against changing it- doing operations on a whole tablespace felt
like it would make sense under 'ALTER TABLESPACE' to me (hence the
implementation) but you're right, it's not actually changing properties
of the tablespaces themselves.

>    MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar

I'm not a huge fan of new top-level constructs and re-using MOVE feels
completely wrong to me as that's used for cursors..

> wouldn't be less confusing.  Not sure what we'd use as command tag
> for it though (not MOVE, since that's taken).

I would have thought something under ALTER TABLE would make more sense,
if we're going to change it, eg:

ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ...

or perhaps something like

ALTER TABLES IN TABLESPACE ...
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
All,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > ALTER TABLESPACE MOVE is a glorified ALTER TABLE.  If ALTER TABLESPACE
> > > MOVE returned ALTER TABLE as a tag, I think it'd work well too; but not
> > > ALTER TABLESPACE.  Individually, since the implementation works by
> > > calling AlterTableInternal(), it already works.
> >
> > > Now if you state that the current design in event_triggers that works by
> > > slicing CommandTag and comparing the pieces is broken, I don't disagree
> > > and I think I have now (in the patch posted in a nearby thread) some
> > > more infrastructure to do it differently.  But even if we do that, I
> > > think we're going to need a way to differentiate ALTER TABLESPACE MOVE
> > > from other forms of ALTER TABLESPACE.  I haven't given this much
> > > thought, though.
> >
> > Yeah, I'd not paid much attention to it either.  Now that I look at it,
> > ALTER TABLESPACE MOVE seems like a pretty unfortunate choice of naming
> > all around, because (unless I'm misunderstanding) it doesn't actually
> > alter any property of the tablespace itself.  It might be a bit late
> > to propose this, but I wonder if some syntax along the lines of
>
> I'm not against changing it- doing operations on a whole tablespace felt
> like it would make sense under 'ALTER TABLESPACE' to me (hence the
> implementation) but you're right, it's not actually changing properties
> of the tablespaces themselves.
>
> >    MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar
>
> I'm not a huge fan of new top-level constructs and re-using MOVE feels
> completely wrong to me as that's used for cursors..
>
> > wouldn't be less confusing.  Not sure what we'd use as command tag
> > for it though (not MOVE, since that's taken).
>
> I would have thought something under ALTER TABLE would make more sense,
> if we're going to change it, eg:
>
> ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ...
>
> or perhaps something like
>
> ALTER TABLES IN TABLESPACE ...

Any further thoughts on this?  I haven't tried to go implement anything
yet but I'm definitely concerned that we may run into a keyword issue
with ALTER TABLE, but I don't really want to add 'TABLES' either.
Anyone have any other suggestions or ideas?
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Stephen Frost wrote:

> Any further thoughts on this?  I haven't tried to go implement anything
> yet but I'm definitely concerned that we may run into a keyword issue
> with ALTER TABLE, but I don't really want to add 'TABLES' either.
> Anyone have any other suggestions or ideas?

I thought that the suggestion to use MOVE was the most appropriate.
However I also concur that re-using MOVE from its current cursor-related
meaning would be troublesome from several PoVs, so if there's a synonym
of MOVE that we can use, that'd probably be better.  I don't like ALTER
TABLES myself; and having an ALTER TABLE command that affects several
tables seems counterintuitive as well.

How about RELOCATE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar
?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
>
> > Any further thoughts on this?  I haven't tried to go implement anything
> > yet but I'm definitely concerned that we may run into a keyword issue
> > with ALTER TABLE, but I don't really want to add 'TABLES' either.
> > Anyone have any other suggestions or ideas?
>
> I thought that the suggestion to use MOVE was the most appropriate.
> However I also concur that re-using MOVE from its current cursor-related
> meaning would be troublesome from several PoVs, so if there's a synonym
> of MOVE that we can use, that'd probably be better.  I don't like ALTER
> TABLES myself; and having an ALTER TABLE command that affects several
> tables seems counterintuitive as well.
>
> How about
>   RELOCATE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar
> ?

The original call for this change was to have it return the command tag
of ALTER TABLE, wasn't it?  Would inventing a new command tag like
RELOCATE actually improve the situation..?  Or is the thought that it'd
still be ALTER TABLE?

That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been
trying to think of a way to put it under that command.  What if we had a
more general way to reference 'all objects in a tablespace'?
"tablespace.*" or "ALL:TABLESAPCE"?  Are there other places which might
benefit from being able to take and operate on all objects in a
tablespace?

Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
pretty darn verbose but is at least a bit more in-line with what we have
done before..
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Tom Lane
Date:
Stephen Frost <sfrost@snowman.net> writes:
> That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been
> trying to think of a way to put it under that command.  What if we had a
> more general way to reference 'all objects in a tablespace'?
> "tablespace.*" or "ALL:TABLESAPCE"?  Are there other places which might
> benefit from being able to take and operate on all objects in a
> tablespace?

> Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
> not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
> pretty darn verbose but is at least a bit more in-line with what we have
> done before..

That's not a bad line of thought --- I doubt that verbosity is critical
here.
        regards, tom lane



Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been
> > trying to think of a way to put it under that command.  What if we had a
> > more general way to reference 'all objects in a tablespace'?
> > "tablespace.*" or "ALL:TABLESAPCE"?  Are there other places which might
> > benefit from being able to take and operate on all objects in a
> > tablespace?
> 
> > Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
> > not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
> > pretty darn verbose but is at least a bit more in-line with what we have
> > done before..
> 
> That's not a bad line of thought --- I doubt that verbosity is critical
> here.

Yep, that works for my purposes.  Also, it seems there should be no
parse conflict because ON is already a reserved keyword ..

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Noah Misch
Date:
On Mon, Jun 23, 2014 at 05:10:02PM -0400, Stephen Frost wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:

> > I'm not against changing it- doing operations on a whole tablespace felt
> > like it would make sense under 'ALTER TABLESPACE' to me (hence the
> > implementation) but you're right, it's not actually changing properties
> > of the tablespaces themselves.
> > 
> > >    MOVE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE foo TO bar
> > 
> > I'm not a huge fan of new top-level constructs and re-using MOVE feels
> > completely wrong to me as that's used for cursors..
> > 
> > > wouldn't be less confusing.  Not sure what we'd use as command tag
> > > for it though (not MOVE, since that's taken).
> > 
> > I would have thought something under ALTER TABLE would make more sense,
> > if we're going to change it, eg:
> > 
> > ALTER TABLE ALL [ TABLES | INDEXES | ... ] IN TABLESPACE SET TABLESPACE ...
> > 
> > or perhaps something like
> > 
> > ALTER TABLES IN TABLESPACE ...
> 
> Any further thoughts on this?  I haven't tried to go implement anything
> yet but I'm definitely concerned that we may run into a keyword issue
> with ALTER TABLE, but I don't really want to add 'TABLES' either.
> Anyone have any other suggestions or ideas?

I recommend:
 SELECT tablespace_move(old_tablespace name, new_tablespace name); SELECT tablespace_move(old_tablespace name,
new_tablespacename, relkind "char");
 

Concerning the problem that started this thread, I would raise one ALTER TABLE
event per table and not fire an event for the bulk request as a whole.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com



Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Noah Misch wrote:

> Concerning the problem that started this thread, I would raise one ALTER TABLE
> event per table and not fire an event for the bulk request as a whole.

Yes, that's how it already works.  Essentially, ALTER TABLESPACE MOVE
calls AlterTableInternal, and that function calls one ATExecFoo() for
each table to be moved.  What my code does is set up "AT event
collection" when ALTER TABLESPACE MOVE is called, and then for each
ATExecFoo() is executed, one event is added to a queue.  When ALTER
TABLESPACE MOVE finishes, all those events are reported together.  This
part is not a problem.  The only part that is a problem is that the
CommandTag comparison in event_trigger.c wasn't happy that a command was
being passed with the ALTER TABLESPACE command tag, because tablespaces
are not supported by that module --- and I think the unhappiness was
only in an assert-only block (but I'm not 100% sure about that last bit,
because I didn't try removing that test.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been
> > trying to think of a way to put it under that command.  What if we had a
> > more general way to reference 'all objects in a tablespace'?
> > "tablespace.*" or "ALL:TABLESAPCE"?  Are there other places which might
> > benefit from being able to take and operate on all objects in a
> > tablespace?
>
> > Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
> > not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
> > pretty darn verbose but is at least a bit more in-line with what we have
> > done before..
>
> That's not a bad line of thought --- I doubt that verbosity is critical
> here.

Alright, sounds like this is more-or-less the concensus.  I'll see about
making it happen shortly.
Thanks!
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > Stephen Frost <sfrost@snowman.net> writes:
> > > That it's more-or-less a bulk 'ALTER TABLE' operation is why I had been
> > > trying to think of a way to put it under that command.  What if we had a
> > > more general way to reference 'all objects in a tablespace'?
> > > "tablespace.*" or "ALL:TABLESAPCE"?  Are there other places which might
> > > benefit from being able to take and operate on all objects in a
> > > tablespace?
> > 
> > > Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
> > > not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
> > > pretty darn verbose but is at least a bit more in-line with what we have
> > > done before..
> > 
> > That's not a bad line of thought --- I doubt that verbosity is critical
> > here.
> 
> Alright, sounds like this is more-or-less the concensus.  I'll see about
> making it happen shortly.

Stephen,

Were you able to work on this?

Can you be more specific on the exact grammar you're considering?  The
proposal above,
ALTER TABLE ON ALL TABLES IN TABLESPACE xyz
doesn't seem very good to me.  I would think it'd be more like
ALTER ALL TABLES IN TABLESPACE xyz
but then if you return ALTER TABLE as a command tag that might be a bit
strange.  Maybe
ALTER TABLE ALL IN TABLESPACE xyz
which AFAICS should work since ALL is already a reserved keyword.


Also, how would we document this?  Would we have it in the same page as
all the ALTER TABLE variants, or would we create a separate page for
ALTER TABLE ALL?  Keeping in mind that in the future we might want to
allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to
have the selection logic documented neatly in its own little page
instead of together with the ALTER TABLE mess which is already rather
large.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > > > Of course, we handle this in 'GRANT' with 'GRANT ON ALL TABLES', so why
> > > > not 'ALTER TABLE ON ALL TABLES IN TABLESPACE <blah>'?  that does get
> > > > pretty darn verbose but is at least a bit more in-line with what we have
> > > > done before..
> > >
> > > That's not a bad line of thought --- I doubt that verbosity is critical
> > > here.
> >
> > Alright, sounds like this is more-or-less the concensus.  I'll see about
> > making it happen shortly.
>
> Were you able to work on this?

Apologies, I've been gone more-or-less all of July.  I'm back now and
have time to spend on this.

> Can you be more specific on the exact grammar you're considering?  The
> proposal above,
> ALTER TABLE ON ALL TABLES IN TABLESPACE xyz
> doesn't seem very good to me.  I would think it'd be more like
> ALTER ALL TABLES IN TABLESPACE xyz
> but then if you return ALTER TABLE as a command tag that might be a bit
> strange.  Maybe
> ALTER TABLE ALL IN TABLESPACE xyz
> which AFAICS should work since ALL is already a reserved keyword.

Interesting idea.  I wonder if we might apply that to other capabilities
of ALTER TABLE too..  That is, make it a generic way to specify that all
tables in the tablespace (or schema..?) should be modified in a certain
way.  We'd have to consider which of the ALTER TABLE operations this
would work for, of course.  I don't believe it'd make sense for any of
the 'ADD/DROP COLUMN' or similar commands, but 'OWNER TO' would make
sense...

I'm not sure how much we want to work this over during beta though..  My
original thought was just to adjust the grammar and hopefully minimize
the other changes in this area, but it'd be good to have an idea about
where we want to take this, so the grammar can support the future
capabilities while not actually adding them at this time.

> Also, how would we document this?  Would we have it in the same page as
> all the ALTER TABLE variants, or would we create a separate page for
> ALTER TABLE ALL?  Keeping in mind that in the future we might want to
> allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to
> have the selection logic documented neatly in its own little page
> instead of together with the ALTER TABLE mess which is already rather
> large.

I would think we'd simply use the ALTER TABLE page as I don't think the
documentation of this would be all that difficult to describe in a
reasonable paragraph.  A page dedicated to it feels like overkill, but
perhaps that might change as we add more.

Other thoughts on this?  We should really get any of the changes we're
doing here done soon.
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Stephen Frost wrote:

> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > Stephen Frost wrote:

> > > Alright, sounds like this is more-or-less the concensus.  I'll see about
> > > making it happen shortly.
> > 
> > Were you able to work on this?
> 
> Apologies, I've been gone more-or-less all of July.  I'm back now and
> have time to spend on this.

Ping?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services



Re: ALTER TABLESPACE MOVE command tag tweak

From
Robert Haas
Date:
On Wed, Aug 13, 2014 at 9:33 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Stephen Frost wrote:
>
>> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
>> > Stephen Frost wrote:
>
>> > > Alright, sounds like this is more-or-less the concensus.  I'll see about
>> > > making it happen shortly.
>> >
>> > Were you able to work on this?
>>
>> Apologies, I've been gone more-or-less all of July.  I'm back now and
>> have time to spend on this.
>
> Ping?

Seriously!

We really should not be making changes of this type less than a month
from our ostensible release date.  That is not enough time for us to
notice if the changes turn out to be not as good as we think they are.
The whole point of beta is to fix things while there's still enough
time for further course correction if needed; if we say, oh, beta's
not totally over yet, I don't have to get my changes in, then it
completely defeats the purpose of having a beta in the first place.

</rant>

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



Re: ALTER TABLESPACE MOVE command tag tweak

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> We really should not be making changes of this type less than a month
> from our ostensible release date.  That is not enough time for us to
> notice if the changes turn out to be not as good as we think they are.

If it weren't for the fact that we'll be wedded forevermore to a bad
choice of syntax, I might agree with you.  But at this point, the
alternatives we have are to fix it now, or fix it never.  I don't
like #2.
        regards, tom lane



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > We really should not be making changes of this type less than a month
> > from our ostensible release date.  That is not enough time for us to
> > notice if the changes turn out to be not as good as we think they are.
>
> If it weren't for the fact that we'll be wedded forevermore to a bad
> choice of syntax, I might agree with you.  But at this point, the
> alternatives we have are to fix it now, or fix it never.  I don't
> like #2.

I'm planning to fix it shortly, as I mentioned to Alvaro on IRC when I
saw his note.
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Robert Haas
Date:
On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> We really should not be making changes of this type less than a month
>> from our ostensible release date.  That is not enough time for us to
>> notice if the changes turn out to be not as good as we think they are.
>
> If it weren't for the fact that we'll be wedded forevermore to a bad
> choice of syntax, I might agree with you.  But at this point, the
> alternatives we have are to fix it now, or fix it never.  I don't
> like #2.

Or postpone the release for another month or two.  There's still a few
other unresolved issues, too, like the problems with psql and expanded
mode; and the JSONB toast problems.  The latter is relatively new, but
we don't have a proposed patch for it yet unless there's on in an
email I haven't read yet, and the former has been lingering for many
months without getting appreciably closer to a resolution.

I like releasing in September as much as anyone, but that contemplates
people taking care to get known issues fixed before the second half of
August.

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



Re: ALTER TABLESPACE MOVE command tag tweak

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If it weren't for the fact that we'll be wedded forevermore to a bad
>> choice of syntax, I might agree with you.  But at this point, the
>> alternatives we have are to fix it now, or fix it never.  I don't
>> like #2.

> Or postpone the release for another month or two.  There's still a few
> other unresolved issues, too, like the problems with psql and expanded
> mode; and the JSONB toast problems.  The latter is relatively new, but
> we don't have a proposed patch for it yet unless there's on in an
> email I haven't read yet, and the former has been lingering for many
> months without getting appreciably closer to a resolution.

Yeah, if we do anything about JSONB we are certainly going to need another
beta release, which means final couldn't be before end of Sept. at the
very earliest.  Still, it's always been project policy that we ship when
it's ready, not when the calendar hits some particular point.

As for the expanded-mode changes, I thought there was consensus to
revert that from 9.4.
        regards, tom lane



Re: ALTER TABLESPACE MOVE command tag tweak

From
Robert Haas
Date:
On Fri, Aug 15, 2014 at 9:57 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Aug 14, 2014 at 6:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> If it weren't for the fact that we'll be wedded forevermore to a bad
>>> choice of syntax, I might agree with you.  But at this point, the
>>> alternatives we have are to fix it now, or fix it never.  I don't
>>> like #2.
>
>> Or postpone the release for another month or two.  There's still a few
>> other unresolved issues, too, like the problems with psql and expanded
>> mode; and the JSONB toast problems.  The latter is relatively new, but
>> we don't have a proposed patch for it yet unless there's on in an
>> email I haven't read yet, and the former has been lingering for many
>> months without getting appreciably closer to a resolution.
>
> Yeah, if we do anything about JSONB we are certainly going to need another
> beta release, which means final couldn't be before end of Sept. at the
> very earliest.  Still, it's always been project policy that we ship when
> it's ready, not when the calendar hits some particular point.

Absolutely.

> As for the expanded-mode changes, I thought there was consensus to
> revert that from 9.4.

Me too.  In fact, I think that's been the consensus for many months,
but unless I'm mistaken it ain't done.

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



Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro, all,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Can you be more specific on the exact grammar you're considering?  The
> proposal above,
> ALTER TABLE ON ALL TABLES IN TABLESPACE xyz
> doesn't seem very good to me.  I would think it'd be more like
> ALTER ALL TABLES IN TABLESPACE xyz
> but then if you return ALTER TABLE as a command tag that might be a bit
> strange.  Maybe
> ALTER TABLE ALL IN TABLESPACE xyz
> which AFAICS should work since ALL is already a reserved keyword.

I've implemented the 'ALTER TABLE ALL IN TABLESPACE ...' appproach, as
discussed.  Patch attached.

> Also, how would we document this?  Would we have it in the same page as
> all the ALTER TABLE variants, or would we create a separate page for
> ALTER TABLE ALL?  Keeping in mind that in the future we might want to
> allow things such as ALTER TABLE ALL IN SCHEMA xyz it might be better to
> have the selection logic documented neatly in its own little page
> instead of together with the ALTER TABLE mess which is already rather
> large.

As mentioned, I'll add this to the ALTER TABLE documentation and remove
it from the TABLESPACE docs.  That's not done yet but I should have time
in the next few days to get that done also and will then commit it all
to master and back-patch to 9.4, barring objections.

    Thanks,

        Stephen

Attachment

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro, all,

* Stephen Frost (sfrost@snowman.net) wrote:
> As mentioned, I'll add this to the ALTER TABLE documentation and remove
> it from the TABLESPACE docs.  That's not done yet but I should have time
> in the next few days to get that done also and will then commit it all
> to master and back-patch to 9.4, barring objections.

Here's a first pass with the documentation changes included.  Feedback
welcome, and I'll review it again and plan to commit it on Tuesday.

    Thanks,

        Stephen

Attachment

Re: ALTER TABLESPACE MOVE command tag tweak

From
Greg Stark
Date:
<p dir="ltr"><br /> On 15 Aug 2014 19:06, "Robert Haas" <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br /> ><br /> > > As for the
expanded-modechanges, I thought there was consensus to<br /> > > revert that from 9.4.<br /> ><br /> > Me
too. In fact, I think that's been the consensus for many months,<br /> > but unless I'm mistaken it ain't done.<br
/><pdir="ltr">Yeah, this is entirely my fault. I was traveling, busy, not feeling well, in various permutations but I
shouldhave gotten to it earlier.<p dir="ltr">I'll take care of it tomorrow morning.<br /> 

Re: ALTER TABLESPACE MOVE command tag tweak

From
Peter Eisentraut
Date:
On 8/17/14 5:19 PM, Stephen Frost wrote:
> Alvaro, all,
> 
> * Stephen Frost (sfrost@snowman.net) wrote:
>> As mentioned, I'll add this to the ALTER TABLE documentation and remove
>> it from the TABLESPACE docs.  That's not done yet but I should have time
>> in the next few days to get that done also and will then commit it all
>> to master and back-patch to 9.4, barring objections.
> 
> Here's a first pass with the documentation changes included.  Feedback
> welcome, and I'll review it again and plan to commit it on Tuesday.

One thing that is not clear from this (before or after) is whether "all
{tables|indexes|etc} in the tablespace" actually means "in this
tablespace and in the current database".  Presumably it does.





Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 8/17/14 5:19 PM, Stephen Frost wrote:
> > Alvaro, all,
> >
> > * Stephen Frost (sfrost@snowman.net) wrote:
> >> As mentioned, I'll add this to the ALTER TABLE documentation and remove
> >> it from the TABLESPACE docs.  That's not done yet but I should have time
> >> in the next few days to get that done also and will then commit it all
> >> to master and back-patch to 9.4, barring objections.
> >
> > Here's a first pass with the documentation changes included.  Feedback
> > welcome, and I'll review it again and plan to commit it on Tuesday.
>
> One thing that is not clear from this (before or after) is whether "all
> {tables|indexes|etc} in the tablespace" actually means "in this
> tablespace and in the current database".  Presumably it does.

Yes, only those in the current database.  There was a mention of that in
the original docs.  I'll add it to the new ones.
Thanks,
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> ALTER TABLE ALL IN TABLESPACE xyz
> which AFAICS should work since ALL is already a reserved keyword.

Pushed to master and REL9_4_STABLE.  Apologies on it taking so long-
things have a bit "interesting" for me over the past month or two. :)
Thanks!
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Fujii Masao
Date:
On Fri, Aug 22, 2014 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
>> ALTER TABLE ALL IN TABLESPACE xyz
>> which AFAICS should work since ALL is already a reserved keyword.
>
> Pushed to master and REL9_4_STABLE.

You seem to have forgotten to update tab-complete.c.
Attached patch updates that.

Regards,

--
Fujii Masao

Attachment

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> You seem to have forgotten to update tab-complete.c.

Good point, absolutely correct.  I have a bad tendency to do that.

> Attached patch updates that.

Many thanks, will review and apply soon.
Thanks again!
    Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:
> On Fri, Aug 22, 2014 at 8:14 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> >> ALTER TABLE ALL IN TABLESPACE xyz
> >> which AFAICS should work since ALL is already a reserved keyword.
> >
> > Pushed to master and REL9_4_STABLE.
>
> You seem to have forgotten to update tab-complete.c.
> Attached patch updates that.

Pushed.  Thanks again,
Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Alvaro Herrera
Date:
Stephen Frost wrote:
> Alvaro,
>
> * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > ALTER TABLE ALL IN TABLESPACE xyz
> > which AFAICS should work since ALL is already a reserved keyword.
>
> Pushed to master and REL9_4_STABLE.

Thanks.  One more tweak --- the whole reason for fiddling with this is
to ensure that event triggers support this operation.  Therefore this
node should be handled by ProcessUtilitySlow, not
standard_ProcessUtility, as in the attached patch.

(I checked the documentation for necessary updates; turns out that the
table in the event triggers chapter says that ddl_command_end etc
support the command "ALTER TABLE", and since this is the tag returned by
the new ALTER TABLE ALL IN TABLESPACE command, there is no update
needed.  In fact, one can argue that the table is wrong currently
because it doesn't say that ALTER TABLE ALL IN TABLESPACE is not
supported.)

I propose this for 9.4 too.

> Apologies on it taking so long-
> things have a bit "interesting" for me over the past month or two. :)

I bet they have!  Have fun,

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > > ALTER TABLE ALL IN TABLESPACE xyz
> > > which AFAICS should work since ALL is already a reserved keyword.
> >
> > Pushed to master and REL9_4_STABLE.
>
> Thanks.  One more tweak --- the whole reason for fiddling with this is
> to ensure that event triggers support this operation.  Therefore this
> node should be handled by ProcessUtilitySlow, not
> standard_ProcessUtility, as in the attached patch.

Ah, right, of course.  I recall looking for what else might need to be
changed and apparently missed that distinction in the call sites.

> (I checked the documentation for necessary updates; turns out that the
> table in the event triggers chapter says that ddl_command_end etc
> support the command "ALTER TABLE", and since this is the tag returned by
> the new ALTER TABLE ALL IN TABLESPACE command, there is no update
> needed.  In fact, one can argue that the table is wrong currently
> because it doesn't say that ALTER TABLE ALL IN TABLESPACE is not
> supported.)

Heh, yes, true.

> I propose this for 9.4 too.

Agreed.  Looks pretty straight-forward, will update soon.

> > Apologies on it taking so long-
> > things have a bit "interesting" for me over the past month or two. :)
>
> I bet they have!  Have fun,

Thanks! :)
Stephen

Re: ALTER TABLESPACE MOVE command tag tweak

From
Stephen Frost
Date:
Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Thanks.  One more tweak --- the whole reason for fiddling with this is
> to ensure that event triggers support this operation.  Therefore this
> node should be handled by ProcessUtilitySlow, not
> standard_ProcessUtility, as in the attached patch.

[...]

> I propose this for 9.4 too.

Done.

> I bet they have!  Have fun,

Thanks!  I'm trying to. :)
Stephen