Thread: ALTER TABLESPACE MOVE command tag tweak
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
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
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
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
* 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
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
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
* 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
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
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
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
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
* 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
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
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
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
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
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
* 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
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
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
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
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
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
<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 />
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.
* 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
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
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
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
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
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
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
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