Thread: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis
Hello,
As Amit Langot pointed out, the column_constraint definition is missing whereas it is used in ALTER TABLE synopsis. It can be easily found in the CREATE TABLE synopsis, but it's not very user friendly.I simply copied/paste the column_constraint definition from the CREATE TABLE synopsis to the ALTER TABLE synopsis. I also had to change the first word "where" to "and" as it's not the first definition in that synopsis.
I choose to add it above table_constraint as column_constraint is used before table_constraint.
Regards,
Lætitia
Attachment
Greetings, * Lætitia Avrot (laetitia.avrot@gmail.com) wrote: > As Amit Langot pointed out, the column_constraint definition is missing > whereas it is used in ALTER TABLE synopsis. It can be easily found in the > CREATE TABLE synopsis, but it's not very user friendly. Agreed. > You will find enclosed my patch. Thanks, this looks pretty reasonable, but did you happen to look for any other keywords in the ALTER TABLE that should really be in ALTER TABLE also? I'm specifically looking at, at least, partition_bound_spec. Maybe you could propose an updated patch which addresses that also, and any other cases you find? Thanks again! Stephen
On 2017/10/31 21:31, Stephen Frost wrote: > * Lætitia Avrot (laetitia.avrot@gmail.com) wrote: >> As Amit Langot pointed out, the column_constraint definition is missing >> whereas it is used in ALTER TABLE synopsis. It can be easily found in the >> CREATE TABLE synopsis, but it's not very user friendly. > > Thanks, this looks pretty reasonable, but did you happen to look for any > other keywords in the ALTER TABLE that should really be in ALTER TABLE > also? > > I'm specifically looking at, at least, partition_bound_spec. Maybe you > could propose an updated patch which addresses that also, and any other > cases you find? Ah, yes. I remember having left out partition_bound_spec simply because I thought it was kind of how it was supposed to be done, seeing that neither column_constraint and table_constraint were expanded in the ALTER TABLE's synopsis. It seems that there are indeed a couple of other things that need to be brought over to ALTER TABLE synopsis including partition_bound_spec. 9f295c08f877 [1] added table_constraint, but missed to add the description of index_parameters and exclude_element which are referenced therein. Attached find updated version of the Lætitia's patch. Thanks, Amit [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=9f295c -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi all,
Thanks Stephen for the suggestion. I wan't thinking globally enough. I was planning to look at it today but Amit was faster. So thanks Amit too!2017-11-01 1:35 GMT+01:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:
On 2017/10/31 21:31, Stephen Frost wrote:
> * Lætitia Avrot (laetitia.avrot@gmail.com) wrote:
>> As Amit Langot pointed out, the column_constraint definition is missing
>> whereas it is used in ALTER TABLE synopsis. It can be easily found in the
>> CREATE TABLE synopsis, but it's not very user friendly.
>
> Thanks, this looks pretty reasonable, but did you happen to look for any
> other keywords in the ALTER TABLE that should really be in ALTER TABLE
> also?
>
> I'm specifically looking at, at least, partition_bound_spec. Maybe you
> could propose an updated patch which addresses that also, and any other
> cases you find?
Ah, yes. I remember having left out partition_bound_spec simply because I
thought it was kind of how it was supposed to be done, seeing that neither
column_constraint and table_constraint were expanded in the ALTER TABLE's
synopsis.
It seems that there are indeed a couple of other things that need to be
brought over to ALTER TABLE synopsis including partition_bound_spec.
9f295c08f877 [1] added table_constraint, but missed to add the description
of index_parameters and exclude_element which are referenced therein.
Attached find updated version of the Lætitia's patch.
Thanks,
Amit
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a= commit;h=9f295c
Greetings Lætitia, Amit, * Lætitia Avrot (laetitia.avrot@gmail.com) wrote: > Thanks Stephen for the suggestion. I wan't thinking globally enough. I was > planning to look at it today but Amit was faster. So thanks Amit too! This seems to have gotten lost in the shuffle, but Amit's patch still applies cleanly and it looks like a good patch to me, so I've added it to the current CF and marked it as Needs Review. There's been some further discussion of this change over in this thread: https://www.postgresql.org/message-id/flat/20171220090816.25744.72592%40wrigleys.postgresql.org#20171220090816.25744.72592@wrigleys.postgresql.org which seemed to reach the conclusion that the proposed patch makes sense. If someone else would like to review it, that'd be great, otherwise I'll probably get it committed soon. Thanks! Stephen
Attachment
On Mon, Jan 15, 2018 at 2:32 PM, Stephen Frost <sfrost@snowman.net> wrote:
> If someone else would like to review it, that'd be great, otherwise I'll
> probably get it committed soon.
FYI the v2 doesn't build:
ref/alter_table.sgml:135: parser error : Opening and ending tag
mismatch: refentry line 6 and synopsis
</synopsis>
^
ref/alter_table.sgml:1534: parser error : chunk is not well balanced
</refentry>
^
ref/alter_table.sgml:1534: parser error : chunk is not well balanced
</refentry>
^
--
Thomas Munro
http://www.enterprisedb.com
On Tue, Jan 23, 2018 at 12:41 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Mon, Jan 15, 2018 at 2:32 PM, Stephen Frost <sfrost@snowman.net> wrote: >> If someone else would like to review it, that'd be great, otherwise I'll >> probably get it committed soon. > > FYI the v2 doesn't build: > > ref/alter_table.sgml:135: parser error : Opening and ending tag > mismatch: refentry line 6 and synopsis > </synopsis> Here's an update to move the new stuff to the correct side of the closing "</synopsis>". Builds for me, and the typesetting looks OK. I'm not sure why the preexisting bogo-productions have inconsistent indentation levels (looking at table_constraint_using_index) but that's not this patch's fault. -- Thomas Munro http://www.enterprisedb.com
Attachment
On 2018/01/23 8:57, Thomas Munro wrote: > On Tue, Jan 23, 2018 at 12:41 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> On Mon, Jan 15, 2018 at 2:32 PM, Stephen Frost <sfrost@snowman.net> wrote: >>> If someone else would like to review it, that'd be great, otherwise I'll >>> probably get it committed soon. >> >> FYI the v2 doesn't build: >> >> ref/alter_table.sgml:135: parser error : Opening and ending tag >> mismatch: refentry line 6 and synopsis >> </synopsis> > > Here's an update to move the new stuff to the correct side of the > closing "</synopsis>". Builds for me, and the typesetting looks OK. > I'm not sure why the preexisting bogo-productions have inconsistent > indentation levels (looking at table_constraint_using_index) but > that's not this patch's fault. Thanks for fixing that. I noticed that partition_bound_spec in the patch is missing hash partition bound syntax that was added after the original patch was written. Fixed in the attached. Thanks, Amit
Attachment
Greetings, * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: > On 2018/01/23 8:57, Thomas Munro wrote: > > On Tue, Jan 23, 2018 at 12:41 PM, Thomas Munro > > <thomas.munro@enterprisedb.com> wrote: > >> On Mon, Jan 15, 2018 at 2:32 PM, Stephen Frost <sfrost@snowman.net> wrote: > >>> If someone else would like to review it, that'd be great, otherwise I'll > >>> probably get it committed soon. > >> > >> FYI the v2 doesn't build: > >> > >> ref/alter_table.sgml:135: parser error : Opening and ending tag > >> mismatch: refentry line 6 and synopsis > >> </synopsis> > > > > Here's an update to move the new stuff to the correct side of the > > closing "</synopsis>". Builds for me, and the typesetting looks OK. > > I'm not sure why the preexisting bogo-productions have inconsistent > > indentation levels (looking at table_constraint_using_index) but > > that's not this patch's fault. > > Thanks for fixing that. > > I noticed that partition_bound_spec in the patch is missing hash partition > bound syntax that was added after the original patch was written. Fixed > in the attached. I've pushed this with just a bit of re-ordering to match the order in which things show up in the synopsis. Thanks! Stephen
Attachment
On 2018/02/02 19:41, Stephen Frost wrote: > * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote: >> On 2018/01/23 8:57, Thomas Munro wrote: >>> Here's an update to move the new stuff to the correct side of the >>> closing "</synopsis>". Builds for me, and the typesetting looks OK. >>> I'm not sure why the preexisting bogo-productions have inconsistent >>> indentation levels (looking at table_constraint_using_index) but >>> that's not this patch's fault. >> >> Thanks for fixing that. >> >> I noticed that partition_bound_spec in the patch is missing hash partition >> bound syntax that was added after the original patch was written. Fixed >> in the attached. > > I've pushed this with just a bit of re-ordering to match the order in > which things show up in the synopsis. Thank you Stephen. Regards, Amit
Hi,
Thank you all for this so warm reception for my very first patch.
Thanks Stephen to have thought to add that patch to the commit fest. As it was committed Friday, I was able to tell the whole story in my talk and that's great.
And thanks again to everyone who helped me with that patch.
Regards
Lætitia
Le 5 févr. 2018 01:36, "Amit Langote" <Langote_Amit_f8@lab.ntt.co.jp
On 2018/02/02 19:41, Stephen Frost wrote:
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> On 2018/01/23 8:57, Thomas Munro wrote:
>>> Here's an update to move the new stuff to the correct side of the
>>> closing "</synopsis>". Builds for me, and the typesetting looks OK.
>>> I'm not sure why the preexisting bogo-productions have inconsistent
>>> indentation levels (looking at table_constraint_using_index) but
>>> that's not this patch's fault.
>>
>> Thanks for fixing that.
>>
>> I noticed that partition_bound_spec in the patch is missing hash partition
>> bound syntax that was added after the original patch was written. Fixed
>> in the attached.
>
> I've pushed this with just a bit of re-ordering to match the order in
> which things show up in the synopsis.
Thank you Stephen.
Regards,
Amit