Обсуждение: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis

Поиск
Список
Период
Сортировка

[HACKERS] Adding column_constraint description in ALTER TABLE synopsis

От
Lætitia Avrot
Дата:
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.

The patch should apply to MASTER (or I messed up with git). I built and tested it successfully on my laptop.

You will find enclosed my patch.

Regards,

Lætitia
Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Stephen Frost
Дата:
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

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Amit Langote
Дата:
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

Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis

От
Lætitia Avrot
Дата:
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!

Have a nice day (UGT),

Lætitia

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

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Stephen Frost
Дата:
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

Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis

От
Thomas Munro
Дата:
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


Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis

От
Thomas Munro
Дата:
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

Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Amit Langote
Дата:
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

Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Stephen Frost
Дата:
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

Вложения

Re: [HACKERS] Adding column_constraint description in ALTER TABLEsynopsis

От
Amit Langote
Дата:
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



Re: [HACKERS] Adding column_constraint description in ALTER TABLE synopsis

От
Lætitia Avrot
Дата:
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> a écrit :
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