Re: GSOC13 proposal - extend RETURNING syntax

Hello,
I'm student who want to participate in Google Summer of Code. I want to
implement feature which allows to get old values directly from update
statement. I mean there should be possibility to choose the value
immedietly before or after update in RETURNING statement. The syntax may
be realized as "aliases". That means: OLD keywordswould be alias to row
before update and NEW to row after update. The conclusion of syntax is:
UPDATE foo SET bar=bar+1 RETURNING OLD.bar AS old_bar, NEW.bar AS new_bar;
UPDATE foo SET ... RETURNING NEW.* will be equivalent to UPDATE foo SET
... RETURNING foo.*
It may be possible to add similar syntax to DELETE and INSERT statements
but I'm not sure if it makes a sense (OLD for DELETE will be alias to
row before delete, NEW for INSERT will be alias to row after insert and
all triggers - however what about NEW for delete and OLD for INSERT?).
Additionally NEW and OLD values will be reserved keywords (it might be
some capability problem since in new PostgreSQL it isn't reserved -
however standard says it is and in old PgSQL it was).
I'd like to hear (read) yours feedback about syntax and/or implement
issues related to this proposal.
Regards,
Karol Trzcionka

On Thu, May 02, 2013 at 11:04:15AM +0200, Karol Trzcionka wrote:
> Hello,
> I'm student who want to participate in Google Summer of Code. I want to
> implement feature which allows to get old values directly from update
> statement. I mean there should be possibility to choose the value
> immedietly before or after update in RETURNING statement. The syntax may
> be realized as "aliases". That means: OLD keywordswould be alias to row
> before update and NEW to row after update. The conclusion of syntax is:
> UPDATE foo SET bar=bar+1 RETURNING OLD.bar AS old_bar, NEW.bar AS new_bar;
> UPDATE foo SET ... RETURNING NEW.* will be equivalent to UPDATE foo SET
> ... RETURNING foo.*
> It may be possible to add similar syntax to DELETE and INSERT statements
> but I'm not sure if it makes a sense (OLD for DELETE will be alias to
> row before delete, NEW for INSERT will be alias to row after insert and
> all triggers - however what about NEW for delete and OLD for INSERT?).
> Additionally NEW and OLD values will be reserved keywords (it might be
> some capability problem since in new PostgreSQL it isn't reserved -
> however standard says it is and in old PgSQL it was).
> I'd like to hear (read) yours feedback about syntax and/or implement
> issues related to this proposal.
> Regards,
> Karol Trzcionka

I would like to include the proposal as we've hammered it out together
on IRC and on GSoC site below.

Cheers,
David.

1. As the SQL standard mandates that OLD and NEW be reserved words, we'll re-reserve them.

2. Let's make OLD and NEW have the same meaning that INSERT/UPDATE/DELETE have when returning rows from the changed table. In particular

INSERT INTO foo (...) RETURNING NEW.*

will be equivalent to

INSERT INTO foo(...) RETURNING foo.*

Similarly for UPDATE and DELETE:

UPDATE foo SET ... RETURNING NEW.*

will be equivalent to

UPDATE foo SET ... RETURNING foo.*

and

DELETE FROM foo ... RETURNING OLD.*

will be equivalent to

DELETE FROM foo ... RETURNING foo.*

As RETURNING clauses have access to everything in the FROM/USING clause, it is important to limit the NEW/OLD rows as being only those in the table being written to in the statement.

3. Let's add an option to UPDATE so that it can RETURN OLD with the same characteristics as above, namely that it refers only to constants and columns in the updated table and not to everything available from the USING clause if included.

--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)org> writes:
> 1. As the SQL standard mandates that OLD and NEW be reserved words, we'll re-reserve them.

As I mentioned yesterday, I'm not exactly thrilled with re-reserving
those, and especially not NEW as it is utterly unnecessary (since the
default would already be to return the NEW column).

It should in any case be possible to do this without converting them to
reserved words; rather the implementation could be that those table
aliases are made available when parsing the UPDATE RETURNING expressions.
(This is, in fact, the way that rules use these names now.) Probably it
should work something like "add these aliases if they don't already
exist in the query", so as to avoid breaking existing applications.

I don't really see a lot of value in hacking the behavior of either
INSERT RETURNING or DELETE RETURNING.

regards, tom lane

W dniu 02.05.2013 17:17, Tom Lane pisze:
> It should in any case be possible to do this without converting them
> to reserved words; rather the implementation could be that those table
> aliases are made available when parsing the UPDATE RETURNING
> expressions. (This is, in fact, the way that rules use these names
> now.) Probably it should work something like "add these aliases if
> they don't already exist in the query", so as to avoid breaking
> existing applications. I don't really see a lot of value in hacking
> the behavior of either INSERT RETURNING or DELETE RETURNING. regards,
> tom lane
I'm not sure about it. If it is not reserved keyword how can user get
old value from table named "old". The new value is not a problem
(doesn't conflict) but what should happened in statement:
UPDATE old SET foo=foo+1 RETURNING old.foo;
If it returns old value, it'll break capability. If it returns new value
(as now), there is no possibility to get old value (and user cries
because of broken feature).
Regards,
Karol Trzcionka

On 2013-05-02 17:32, Karol Trzcionka wrote:
> W dniu 02.05.2013 17:17, Tom Lane pisze:
>> It should in any case be possible to do this without converting them
>> to reserved words; rather the implementation could be that those table
>> aliases are made available when parsing the UPDATE RETURNING
>> expressions. (This is, in fact, the way that rules use these names
>> now.) Probably it should work something like "add these aliases if
>> they don't already exist in the query", so as to avoid breaking
>> existing applications. I don't really see a lot of value in hacking
>> the behavior of either INSERT RETURNING or DELETE RETURNING.
>
> what should happened in statement:
> UPDATE old SET foo=foo+1 RETURNING old.foo;
> If it returns old value, it'll break capability. If it returns new value
> (as now), there is no possibility to get old value (and user cries
> because of broken feature).

In Tom's proposal that would give you the "new" value.

Personally I would maybe prefer reserving NEW/OLD, but if we go through
with Tom's idea, this should work:

UPDATE old myold SET foo=foo+1 RETURNING myold.foo, old.foo;

What I'm more interested in is: how can we make this feature work in
PL/PgSQL where OLD means something different?

Regards,
Marko Tiikkaja

Marko Tiikkaja <marko(at)joh(dot)to> writes:
> What I'm more interested in is: how can we make this feature work in
> PL/PgSQL where OLD means something different?

That's a really good point: if you follow this approach then you're
creating fundamental conflicts for use of the feature in trigger
functions or rules, which will necessarily have conflicting uses of
those names. Yeah, we could define scoping rules such that there's
an unambiguous interpretation, but then the user is just out of luck
if he wants to reference the other definition. (This problem is
probably actually worse if you implement with reserved words rather
than aliases.)

I'm thinking it would be better to invent some other notation for access
to old-row values.

regards, tom lane

On 2013-05-02 12:23:19 -0400, Tom Lane wrote:
> Marko Tiikkaja <marko(at)joh(dot)to> writes:
> > What I'm more interested in is: how can we make this feature work in
> > PL/PgSQL where OLD means something different?
>
> That's a really good point: if you follow this approach then you're
> creating fundamental conflicts for use of the feature in trigger
> functions or rules, which will necessarily have conflicting uses of
> those names. Yeah, we could define scoping rules such that there's
> an unambiguous interpretation, but then the user is just out of luck
> if he wants to reference the other definition. (This problem is
> probably actually worse if you implement with reserved words rather
> than aliases.)
>
> I'm thinking it would be better to invent some other notation for access
> to old-row values.

prior/after? Both are unreserved keywords atm and it seems far less
likely to have conflicts than new/old.

Greetings,

Andres Freund

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

On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote:
> On 2013-05-02 12:23:19 -0400, Tom Lane wrote:
> > Marko Tiikkaja <marko(at)joh(dot)to> writes:
> > > What I'm more interested in is: how can we make this feature work in
> > > PL/PgSQL where OLD means something different?
> >
> > That's a really good point: if you follow this approach then you're
> > creating fundamental conflicts for use of the feature in trigger
> > functions or rules, which will necessarily have conflicting uses of
> > those names. Yeah, we could define scoping rules such that there's
> > an unambiguous interpretation, but then the user is just out of luck
> > if he wants to reference the other definition. (This problem is
> > probably actually worse if you implement with reserved words rather
> > than aliases.)
> >
> > I'm thinking it would be better to invent some other notation for access
> > to old-row values.
>
> prior/after? Both are unreserved keywords atm and it seems far less
> likely to have conflicts than new/old.

BEFORE/AFTER seems more logical to me. Yes, those words both have
meaning in, for example, a trigger definition, but they're clearly
separable by context.

Yay, bike-shedding!

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2013/5/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>

> Marko Tiikkaja <marko(at)joh(dot)to> writes:
> > What I'm more interested in is: how can we make this feature work in
> > PL/PgSQL where OLD means something different?
>
> That's a really good point: if you follow this approach then you're
> creating fundamental conflicts for use of the feature in trigger
> functions or rules, which will necessarily have conflicting uses of
> those names. Yeah, we could define scoping rules such that there's
> an unambiguous interpretation, but then the user is just out of luck
> if he wants to reference the other definition. (This problem is
> probably actually worse if you implement with reserved words rather
> than aliases.)
>
> I'm thinking it would be better to invent some other notation for access
> to old-row values.
>

I am not sure, but I am thinking so NEW and OLD are used in some statements
and features ANSI SQL 2012, so probably we should to do keywords from these
words if we would to support modern ANSI SQL

Regards

Pavel

>
> regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

David Fetter <david(at)fetter(dot)org> writes:
> On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote:
>> prior/after? Both are unreserved keywords atm and it seems far less
>> likely to have conflicts than new/old.

> BEFORE/AFTER seems more logical to me.

Works for me.

regards, tom lane

W dniu 02.05.2013 19:40, Tom Lane pisze:
>> BEFORE/AFTER seems more logical to me.
> Works for me.
>
What do you think about function- or cast-like syntax. I mean:
RETURNING OLD(foo.bar)
or RETURNING OLD(foo).bar
or RETURNING (foo::OLD).bar ?
I think none of them should conflict with any other statements.
Regards,
Karol Trzcionka

On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)org> writes:
> > On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote:
> >> prior/after? Both are unreserved keywords atm and it seems far less
> >> likely to have conflicts than new/old.
>
> > BEFORE/AFTER seems more logical to me.
>
> Works for me.
>
> regards, tom lane

Maybe we can make BEFORE and AFTER implied aliases rather than
keywords. What say?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Sent from my iPad

On 03-May-2013, at 0:07, David Fetter <david(at)fetter(dot)org> wrote:

> On Thu, May 02, 2013 at 01:40:59PM -0400, Tom Lane wrote:
>> David Fetter <david(at)fetter(dot)org> writes:
>>> On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote:
>>>> prior/after? Both are unreserved keywords atm and it seems far less
>>>> likely to have conflicts than new/old.
>>
>>> BEFORE/AFTER seems more logical to me.
>>
>> Works for me.
>>
>> regards, tom lane
>
> Maybe we can make BEFORE and AFTER implied aliases rather than
> keywords. What say?
>
>

I agree.Overall,I like the concept.

Regards,

Atri

David Fetter <david(at)fetter(dot)org> writes:
> Maybe we can make BEFORE and AFTER implied aliases rather than
> keywords. What say?

Well, they still have to be unreserved keywords for their use in
trigger-related commands, but as far as this feature is concerned
I think they're best off being handled as automatically-supplied
aliases. IOW the patch needn't touch gram.y at all.

regards, tom lane

Karol Trzcionka <karlikt(at)gmail(dot)com> writes:
> What do you think about function- or cast-like syntax. I mean:
> RETURNING OLD(foo.bar)
> or RETURNING OLD(foo).bar
> or RETURNING (foo::OLD).bar ?
> I think none of them should conflict with any other statements.

I think you'll find those alternatives are at least as ugly to
implement as they are to look at ...

regards, tom lane

W dniu 02.05.2013 20:45, Tom Lane pisze:
> David Fetter <david(at)fetter(dot)org> writes:
>> Maybe we can make BEFORE and AFTER implied aliases rather than
>> keywords. What say?
> Well, they still have to be unreserved keywords for their use in
> trigger-related commands, but as far as this feature is concerned
> I think they're best off being handled as automatically-supplied
> aliases. IOW the patch needn't touch gram.y at all.
Well... the syntax which wouldn't conflict with Pl/PgSQL is (draft):
INSERT INTO foo ... RETURNING AFTER.*
UPDATE foo SET ... RETURNING AFTER.*, BEFORE.*
DELETE FROM foo ... RETURNING BEFORE.*
It will not solve the problems:
1. How to access to old rows if the table is named "BEFORE"?
2. Should AFTER for DELETE and BEFORE for INSERT be allowed? If yes what
should they return? I mean: what should be result of:
INSERT INTO foo ... RETURNING BEFORE.*
and
DELETE FROM foo ... RETURNING AFTER.*
?
The best summarize of dropping NEW/OLD keywords for this proposal was
proposed while IRC meeting:
create or replace function ft1(integer) returns integer language plpgsql
as $f$ <<x>> declare r rt1; begin select 1 as a into r; update rt1 r set
a = a + 1 returning XXX into r; raise notice 'r = %', r; return null;
end; $f$;
Regards,
Karol Trzcionka

On 03/05/13 04:52, David Fetter wrote:
> On Thu, May 02, 2013 at 06:28:53PM +0200, Andres Freund wrote:
>> On 2013-05-02 12:23:19 -0400, Tom Lane wrote:
>>> Marko Tiikkaja <marko(at)joh(dot)to> writes:
>>>> What I'm more interested in is: how can we make this feature work in
>>>> PL/PgSQL where OLD means something different?
>>> That's a really good point: if you follow this approach then you're
>>> creating fundamental conflicts for use of the feature in trigger
>>> functions or rules, which will necessarily have conflicting uses of
>>> those names. Yeah, we could define scoping rules such that there's
>>> an unambiguous interpretation, but then the user is just out of luck
>>> if he wants to reference the other definition. (This problem is
>>> probably actually worse if you implement with reserved words rather
>>> than aliases.)
>>>
>>> I'm thinking it would be better to invent some other notation for access
>>> to old-row values.
>> prior/after? Both are unreserved keywords atm and it seems far less
>> likely to have conflicts than new/old.
> BEFORE/AFTER seems more logical to me. Yes, those words both have
> meaning in, for example, a trigger definition, but they're clearly
> separable by context.
>
> Yay, bike-shedding!
>
> Cheers,
> David.
I prefer 'PRIOR & 'AFTER' as the both have the same length
- but perhaps that is just me! :-)

Cheers,
Gavin

W dniu 02.05.2013 23:34, Gavin Flower pisze:
> I prefer 'PRIOR & 'AFTER' as the both have the same length
> - but perhaps that is just me! :-)
I think it doesn't matter at now because PRIOR has the same problems as
BEFORE ;)
Regards,
Karol

Karol Trzcionka <karlikt(at)gmail(dot)com> writes:
> It will not solve the problems:
> 1. How to access to old rows if the table is named "BEFORE"?

The user can simply choose to use a different table alias, as Andres
explained upthread. If any table's active alias is "before" (or
"after"), we just don't activate the corresponding implicit alias.

> 2. Should AFTER for DELETE and BEFORE for INSERT be allowed? If yes what
> should they return?

These cases should certainly fail. Now, IMO there's no very good reason
to alter the behavior at all for INSERT/DELETE; only UPDATE has an issue
here. But if we were going to support the extra aliases in those
commands, only the ones that actually make sense should be allowed.

regards, tom lane

Hello,
according to my mentor's suggestion, I send first PoC patch of
"RETURNING AFTER/BEFORE" statement. Some info:
- it is early version - more hack PoC than RC patch
- AFTER in this version works as decribed before but it returns row
after update but before post-update before (to be fixed or switch with
current "*" behaviour - I don't know yet)
- patch passes all regression tests
- the code is uncommented already, to be fixed
I'm not sure what may fail so you use it on your risk ;)
Regards,
Karol

This is a proposal to implement functionalities for the handling of
National Characters.

[Introduction]

The aim of this proposal is to eventually have a way to represent
'National Characters' in a uniform way, even in non-UTF8 encoded
databases. Many of our customers in the Asian region who are now, as
part of their platform modernization, are moving away from mainframes
where they have used National Characters representation in COBOL and
other databases. Having stronger support for national characters
representation will also make it easier for these customers to look at
PostgreSQL more favourably when migrating from other well known RDBMSs
who all have varying degrees of NCHAR/NVARCHAR support.

[Specifications]

Broadly speaking, the national characters implementation ideally will
include the following
- Support for NCHAR/NVARCHAR data types
- Representing NCHAR and NVARCHAR columns in UTF-8 encoding in non-UTF8
databases
- Support for UTF16 column encoding and representing NCHAR and NVARCHAR
columns in UTF16 encoding in all databases.
- Support for NATIONAL_CHARACTER_SET GUC variable that will determine
the encoding that will be used in NCHAR/NVARCHAR columns.

The above points are at the moment a 'wishlist' only. Our aim is to
tackle them one-by-one as we progress. I will send a detailed proposal
later with more technical details.

The main aim at the moment is to get some feedback on the above to know
if this feature is something that would benefit PostgreSQL in general,
and if users maintaining DBs in non-English speaking regions will find
this beneficial.

Rgds,
Arul Shaji

P.S.: It has been quite some time since I send a correspondence to this
list. Our mail server adds a standard legal disclaimer to all outgoing
mails, which I know that this list is not a huge fan of. I used to have
an exemption for the mails I send to this list. If the disclaimer
appears, apologies in advance. I will rectify that on the next one.

Arul Shaji,

NCHAR support is on our TODO list for some time and I would like to
welcome efforts trying to implement it. However I have a few
questions:

> This is a proposal to implement functionalities for the handling of
> National Characters.
>
> [Introduction]
>
> The aim of this proposal is to eventually have a way to represent
> 'National Characters' in a uniform way, even in non-UTF8 encoded
> databases. Many of our customers in the Asian region who are now, as
> part of their platform modernization, are moving away from mainframes
> where they have used National Characters representation in COBOL and
> other databases. Having stronger support for national characters
> representation will also make it easier for these customers to look at
> PostgreSQL more favourably when migrating from other well known RDBMSs
> who all have varying degrees of NCHAR/NVARCHAR support.
>
> [Specifications]
>
> Broadly speaking, the national characters implementation ideally will
> include the following
> - Support for NCHAR/NVARCHAR data types
> - Representing NCHAR and NVARCHAR columns in UTF-8 encoding in non-UTF8
> databases

I think this is not a trivial work because we do not have framework to
allow mixed encodings in a database. I'm interested in how you are
going to solve the problem.

> - Support for UTF16 column encoding and representing NCHAR and NVARCHAR
> columns in UTF16 encoding in all databases.

Why do yo need UTF-16 as the database encoding? UTF-8 is already
supported, and any UTF-16 character can be represented in UTF-8 as far
as I know.

> - Support for NATIONAL_CHARACTER_SET GUC variable that will determine
> the encoding that will be used in NCHAR/NVARCHAR columns.

You said NCHAR's encoding is UTF-8. Why do you need the GUC if NCHAR's
encoding is fixed to UTF-8?

> The above points are at the moment a 'wishlist' only. Our aim is to
> tackle them one-by-one as we progress. I will send a detailed proposal
> later with more technical details.
>
> The main aim at the moment is to get some feedback on the above to know
> if this feature is something that would benefit PostgreSQL in general,
> and if users maintaining DBs in non-English speaking regions will find
> this beneficial.
>
> Rgds,
> Arul Shaji
>
>
>
> P.S.: It has been quite some time since I send a correspondence to this
> list. Our mail server adds a standard legal disclaimer to all outgoing
> mails, which I know that this list is not a huge fan of. I used to have
> an exemption for the mails I send to this list. If the disclaimer
> appears, apologies in advance. I will rectify that on the next one.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Fri, Jul 5, 2013 at 2:02 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
>> - Support for NATIONAL_CHARACTER_SET GUC variable that will determine
>> the encoding that will be used in NCHAR/NVARCHAR columns.
>
> You said NCHAR's encoding is UTF-8. Why do you need the GUC if NCHAR's
> encoding is fixed to UTF-8?

Not only that, but I don't think it can be a GUC. Maybe a compile-time
switch, but if it were a GUC, how do you handle an existing database
in UTF-8 when the setting is switched to UTF-16? Re-encode everything?
Store the encoding along each value? It's a mess.

Either fix it at UTF-8, or make it a compile-time thing, I'd say.

Ishii san,

Thank you for your positive and early response.

> -----Original Message-----
> From: Tatsuo Ishii [mailto:ishii(at)postgresql(dot)org]
> Sent: Friday, 5 July 2013 3:02 PM
> To: Arulappan, Arul Shaji
> Cc: pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] Proposal - Support for National Characters
> functionality
>
> Arul Shaji,
>
> NCHAR support is on our TODO list for some time and I would like to
welcome
> efforts trying to implement it. However I have a few
> questions:
>
> > This is a proposal to implement functionalities for the handling of
> > National Characters.
> >
> > [Introduction]
> >
> > The aim of this proposal is to eventually have a way to represent
> > 'National Characters' in a uniform way, even in non-UTF8 encoded
> > databases. Many of our customers in the Asian region who are now, as
> > part of their platform modernization, are moving away from
mainframes
> > where they have used National Characters representation in COBOL and
> > other databases. Having stronger support for national characters
> > representation will also make it easier for these customers to look
at
> > PostgreSQL more favourably when migrating from other well known
RDBMSs
> > who all have varying degrees of NCHAR/NVARCHAR support.
> >
> > [Specifications]
> >
> > Broadly speaking, the national characters implementation ideally
will
> > include the following
> > - Support for NCHAR/NVARCHAR data types
> > - Representing NCHAR and NVARCHAR columns in UTF-8 encoding in
> > non-UTF8 databases
>
> I think this is not a trivial work because we do not have framework to
allow
> mixed encodings in a database. I'm interested in how you are going to
solve
> the problem.
>

I would be lying if I said I have the design already speced out. I will
be working on this in the coming weeks and hope to design a working
solution consulting with the community.

> > - Support for UTF16 column encoding and representing NCHAR and
> > NVARCHAR columns in UTF16 encoding in all databases.
>
> Why do yo need UTF-16 as the database encoding? UTF-8 is already
supported,
> and any UTF-16 character can be represented in UTF-8 as far as I know.
>

Yes, that's correct. However there are advantages in using UTF-16
encoding for those characters that are always going to take atleast
two-bytes to represent.

Having said that, my intention is to use UTF-8 for NCHAR as well.
Supporting UTF-16 will be even more complicated as it is not supported
natively in some Linux platforms. I only included it to give an option.

> > - Support for NATIONAL_CHARACTER_SET GUC variable that will
determine
> > the encoding that will be used in NCHAR/NVARCHAR columns.
>
> You said NCHAR's encoding is UTF-8. Why do you need the GUC if NCHAR's
> encoding is fixed to UTF-8?
>

If we are going to only support UTF-8 for NCHAR, then we don't need the
GUC variable obviously.

Rgds,
Arul Shaji

> > The above points are at the moment a 'wishlist' only. Our aim is to
> > tackle them one-by-one as we progress. I will send a detailed
proposal
> > later with more technical details.
> >
> > The main aim at the moment is to get some feedback on the above to
> > know if this feature is something that would benefit PostgreSQL in
> > general, and if users maintaining DBs in non-English speaking
regions
> > will find this beneficial.
> >
> > Rgds,
> > Arul Shaji
> >
> >
> >
> > P.S.: It has been quite some time since I send a correspondence to
> > this list. Our mail server adds a standard legal disclaimer to all
> > outgoing mails, which I know that this list is not a huge fan of. I
> > used to have an exemption for the mails I send to this list. If the
> > disclaimer appears, apologies in advance. I will rectify that on the
next
> one.
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp

> -----Original Message-----
> From: Claudio Freire [mailto:klaussfreire(at)gmail(dot)com]
> Sent: Friday, 5 July 2013 3:41 PM
> To: Tatsuo Ishii
> Cc: Arulappan, Arul Shaji; PostgreSQL-Dev
> Subject: Re: [HACKERS] Proposal - Support for National Characters
> functionality
>
> On Fri, Jul 5, 2013 at 2:02 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org>
wrote:
> >> - Support for NATIONAL_CHARACTER_SET GUC variable that will
determine
> >> the encoding that will be used in NCHAR/NVARCHAR columns.
> >
> > You said NCHAR's encoding is UTF-8. Why do you need the GUC if
NCHAR's
> > encoding is fixed to UTF-8?
>
>
> Not only that, but I don't think it can be a GUC. Maybe a compile-time
switch,
> but if it were a GUC, how do you handle an existing database in UTF-8
when the
> setting is switched to UTF-16? Re-encode everything?
> Store the encoding along each value? It's a mess.
>
> Either fix it at UTF-8, or make it a compile-time thing, I'd say.

Agreed, that to begin with we only support UTF-8 encoding for NCHAR
columns. If that is the case, do we still need a compile time option to
turn on/off NCHAR functionality ? ?

Rgds,
Arul Shaji

On 07/05/2013 02:12 AM, Arulappan, Arul Shaji wrote:
> - Support for UTF16 column encoding and representing NCHAR and
> NVARCHAR columns in UTF16 encoding in all databases.
>> Why do yo need UTF-16 as the database encoding? UTF-8 is already
> supported,
>> and any UTF-16 character can be represented in UTF-8 as far as I know.
>>
> Yes, that's correct. However there are advantages in using UTF-16
> encoding for those characters that are always going to take atleast
> two-bytes to represent.
>

Any suggestion to store data in utf-16 is likely to be a complete
non-starter. I suggest you research our previously stated requirements
for server side encodings.

cheers

andrew

On Fri, Jul 05, 2013 at 03:22:33AM +0200, Karol Trzcionka wrote:
> Hello,
> according to my mentor's suggestion, I send first PoC patch of
> "RETURNING AFTER/BEFORE" statement. Some info:
> - it is early version - more hack PoC than RC patch
> - AFTER in this version works as decribed before but it returns row
> after update but before post-update before (to be fixed or switch with
> current "*" behaviour - I don't know yet)
> - patch passes all regression tests
> - the code is uncommented already, to be fixed
> I'm not sure what may fail so you use it on your risk ;)
> Regards,
> Karol

Karol,

Per discussion in IRC, please follow up with your patch that includes
such documentation, new regression tests, etc., you've written for the
feature.

Thanks! :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 7/4/13 10:11 PM, Arulappan, Arul Shaji wrote:
> The main aim at the moment is to get some feedback on the above to know
> if this feature is something that would benefit PostgreSQL in general,
> and if users maintaining DBs in non-English speaking regions will find
> this beneficial.

For European languages, I think everyone has moved to using Unicode, so
the demand for supporting multiple encodings is approaching zero. The
CJK realm might have difference requirements.

Yes, what I know almost all use utf8 without problems. Long time I didn't
see any request for multi encoding support.
Dne 5.7.2013 20:28 "Peter Eisentraut" <peter_e(at)gmx(dot)net> napsal(a):

> On 7/4/13 10:11 PM, Arulappan, Arul Shaji wrote:
> > The main aim at the moment is to get some feedback on the above to know
> > if this feature is something that would benefit PostgreSQL in general,
> > and if users maintaining DBs in non-English speaking regions will find
> > this beneficial.
>
> For European languages, I think everyone has moved to using Unicode, so
> the demand for supporting multiple encodings is approaching zero. The
> CJK realm might have difference requirements.
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Updated patch:
- include sgml
- fix all compiler warnings
- some cleanup
- fix correctness of feature
Regards,
Karol

Next version:
- cleanup
- regression test
- fix issue reported by johto (invalid values in parallel transactions)
I would like more feedback and comments about the patch, as some parts
may be too hacky.
In particular, is it a problem that I update a pointer to planSlot? In
my patch, it points to tuple after all updates done between planner and
executor (in fact it is not planSlot right now). I don't know whether
the tuple could be deleted in the intervening time and if the pointer
doesn't point to "unreserved" memory (I mean - memory which may be
overwritten by something meanwhile).
Regards,
Karol

On Sat, Jul 13, 2013 at 12:49:45AM +0200, Karol Trzcionka wrote:
> Next version:
> - cleanup
> - regression test
> - fix issue reported by johto (invalid values in parallel transactions)
> I would like more feedback and comments about the patch, as some parts
> may be too hacky.
> In particular, is it a problem that I update a pointer to planSlot? In
> my patch, it points to tuple after all updates done between planner and
> executor (in fact it is not planSlot right now). I don't know whether
> the tuple could be deleted in the intervening time and if the pointer
> doesn't point to "unreserved" memory (I mean - memory which may be
> overwritten by something meanwhile).

Thanks for the updated patch!

Anybody care to look this over for vulnerabilities as described above?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Fri, Jul 5, 2013 at 2:35 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Yes, what I know almost all use utf8 without problems. Long time I didn't
> see any request for multi encoding support.

Well, not *everything* can be represented as UTF-8; I think this is
particularly an issue with Asian languages.

If we chose to do it, I think that per-column encoding support would
end up looking a lot like per-column collation support: it would be
yet another per-column property along with typoid, typmod, and
typcollation. I'm not entirely sure it's worth it, although FWIW I do
believe Oracle has something like this. At any rate, it seems like
quite a lot of work.

Another idea would be to do something like what we do for range types
- i.e. allow a user to declare a type that is a differently-encoded
version of some base type. But even that seems pretty hard.

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

>
> On Fri, Jul 5, 2013 at 2:35 PM, Pavel Stehule
<pavel(dot)stehule(at)gmail(dot)com> wrote:
> > Yes, what I know almost all use utf8 without problems. Long time I
> > didn't see any request for multi encoding support.
>
> Well, not *everything* can be represented as UTF-8; I think this is
> particularly an issue with Asian languages.
>
> If we chose to do it, I think that per-column encoding support would
end up
> looking a lot like per-column collation support: it would be yet
another per-
> column property along with typoid, typmod, and typcollation. I'm not
entirely
> sure it's worth it, although FWIW I do believe Oracle has something
like this.

Yes, the idea is that users will be able to declare columns of type
NCHAR or NVARCHAR which will use the pre-determined encoding type. If we
say that NCHAR is UTF-8 then the NCHAR column will be of UTF-8 encoding
irrespective of the database encoding. It will be up to us to restrict
what Unicode encodings we want to support for NCHAR/NVARCHAR columns.
This is based on my interpretation of the SQL standard. As you allude to
above, Oracle has a similar behaviour (they support UTF-16 as well).

Support for UTF-16 will be difficult without linking with some external
libraries such as ICU.

> At any rate, it seems like quite a lot of work.

Thanks for putting my mind at ease ;-)

Rgds,
Arul Shaji

>
> Another idea would be to do something like what we do for range types
> - i.e. allow a user to declare a type that is a differently-encoded
version of
> some base type. But even that seems pretty hard.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL
Company

On Mon, Jul 15, 2013 at 4:37 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 5, 2013 at 2:35 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Yes, what I know almost all use utf8 without problems. Long time I didn't
>> see any request for multi encoding support.
>
> Well, not *everything* can be represented as UTF-8; I think this is
> particularly an issue with Asian languages.

What cannot be represented as UTF-8? UTF-8 can represent every
character in the Unicode character set, whereas UTF-16 can encode
characters 0 to 0x10FFFF.

Does support for alternative multi-byte encodings have something to do
with the Han unification controversy? I don't know terribly much about
this, so apologies if that's just wrong.

--
Peter Geoghegan

>> On Fri, Jul 5, 2013 at 2:35 PM, Pavel Stehule
> <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> > Yes, what I know almost all use utf8 without problems. Long time I
>> > didn't see any request for multi encoding support.
>>
>> Well, not *everything* can be represented as UTF-8; I think this is
>> particularly an issue with Asian languages.
>>
>> If we chose to do it, I think that per-column encoding support would
> end up
>> looking a lot like per-column collation support: it would be yet
> another per-
>> column property along with typoid, typmod, and typcollation. I'm not
> entirely
>> sure it's worth it, although FWIW I do believe Oracle has something
> like this.
>
> Yes, the idea is that users will be able to declare columns of type
> NCHAR or NVARCHAR which will use the pre-determined encoding type. If we
> say that NCHAR is UTF-8 then the NCHAR column will be of UTF-8 encoding
> irrespective of the database encoding. It will be up to us to restrict
> what Unicode encodings we want to support for NCHAR/NVARCHAR columns.
> This is based on my interpretation of the SQL standard. As you allude to
> above, Oracle has a similar behaviour (they support UTF-16 as well).
>
> Support for UTF-16 will be difficult without linking with some external
> libraries such as ICU.

Can you please elaborate more on this? Why do you exactly need ICU?

Also I don't understand why you need UTF-16 support as a database
encoding because UTF-8 and UTF-16 are logically equivalent, they are
just different represention (encoding) of Unicode. That means if we
already support UTF-8 (I'm sure we already do), there's no particular
reason we need to add UTF-16 support.

Maybe you just want to support UTF-16 as a client encoding?
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On Mon, Jul 15, 2013 at 8:58 AM, Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> Also I don't understand why you need UTF-16 support as a database
> encoding because UTF-8 and UTF-16 are logically equivalent, they are
> just different represention (encoding) of Unicode. That means if we
> already support UTF-8 (I'm sure we already do), there's no particular
> reason we need to add UTF-16 support.

To be fair, there is a small reason to support UTF-16 even with UTF-8
available. I personally do not find it compelling, but perhaps I am
not best placed to judge such things. As Wikipedia says on the the
English UTF-8 article:

"Characters U+0800 through U+FFFF use three bytes in UTF-8, but only
two in UTF-16. As a result, text in (for example) Chinese, Japanese or
Hindi could take more space in UTF-8 if there are more of these
characters than there are ASCII characters. This happens for pure text
but rarely for HTML documents. For example, both the Japanese UTF-8
and the Hindi Unicode articles on Wikipedia take more space in UTF-16
than in UTF-8."

This is the only advantage of UTF-16 over UTF-8 as a server encoding.
I'm inclined to take the fact that there has been so few (no?)
complaints from PostgreSQL's large Japanese user-base about the lack
of UTF-16 support as suggesting that that isn't considered to be a
compelling feature in the CJK realm.

--
Peter Geoghegan

> On Mon, Jul 15, 2013 at 4:37 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jul 5, 2013 at 2:35 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> Yes, what I know almost all use utf8 without problems. Long time I didn't
>>> see any request for multi encoding support.
>>
>> Well, not *everything* can be represented as UTF-8; I think this is
>> particularly an issue with Asian languages.
>
> What cannot be represented as UTF-8? UTF-8 can represent every
> character in the Unicode character set, whereas UTF-16 can encode
> characters 0 to 0x10FFFF.
>
> Does support for alternative multi-byte encodings have something to do
> with the Han unification controversy? I don't know terribly much about
> this, so apologies if that's just wrong.

There's a famous problem regarding conversion between Unicode and other
encodings, such as Shift Jis.

There are lots of discussion on this. Here is the one from Microsoft:

http://support.microsoft.com/kb/170559/EN-US
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

On 7/15/13 1:26 AM, Arulappan, Arul Shaji wrote:
> Yes, the idea is that users will be able to declare columns of type
> NCHAR or NVARCHAR which will use the pre-determined encoding type. If we
> say that NCHAR is UTF-8 then the NCHAR column will be of UTF-8 encoding
> irrespective of the database encoding. It will be up to us to restrict
> what Unicode encodings we want to support for NCHAR/NVARCHAR columns.

I would try implementing this as an extension at first, with a new data
type that is internally encoded differently. We have citext as
precedent for successfully implementing text-like data types in user space.

On Mon, Jul 15, 2013 at 05:11:40PM +0900, Tatsuo Ishii wrote:
> > Does support for alternative multi-byte encodings have something to do
> > with the Han unification controversy? I don't know terribly much about
> > this, so apologies if that's just wrong.
>
> There's a famous problem regarding conversion between Unicode and other
> encodings, such as Shift Jis.
>
> There are lots of discussion on this. Here is the one from Microsoft:
>
> http://support.microsoft.com/kb/170559/EN-US

Apart from Shift-JIS not being a well defined (it's more a family of
encodings) it has the unusual feature of providing multiple ways to
encode the same character. This is not even a Han unification issue,
they have largely been addressed. For example, the square-root symbol
exists twice (0x8795 and 0x81E3) and many other mathmatical symbols
also.

Here's the code page which you can browse online:

http://msdn.microsoft.com/en-us/goglobal/cc305152

Which means to be round-trippable Unicode would have to double those
characters, but this would make it hard/impossible to round-trip with
any other character set that had those characters. No easy solution
here.

Something that has been done before [1] is to map the doubles to the
custom area of the unicode space (0xe000-0xffff). It gives you
round-trip support at the expense of having to handle those characters
yourself. But since postgres doesn't do anything meaningful with
unicode characters this might be acceptable.

[1] Python does a similar trick to handle filenames coming from disk in
an unknown encoding:
http://docs.python.org/3/howto/unicode.html#files-in-an-unknown-encoding

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

New version:
- fix returning "after" values if there are not "before"
- add more regression tests
I'd like to hear/read any feedback ;)
Regards,
Karol

I've noticed problem with "UPDATE ... FROM" statement. Fix in new version.
Regards,
Karol

On Mon, Jul 22, 2013 at 09:52:14PM +0200, Karol Trzcionka wrote:
> I've noticed problem with "UPDATE ... FROM" statement. Fix in new version.
> Regards,
> Karol

What problem or problems did you notice, and what did you change to
fix them?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

W dniu 23.07.2013 06:22, David Fetter pisze:
> What problem or problems did you notice, and what did you change to
> fix them?
"UPDATE ... FROM" generated "ERROR: variable not found in subplan
target lists". I've added some workaround in add_vars_to_targetlist:
- if it is an "after" - simple change var->varno to base RTE (it should
always exists, the value is temporary, it will change to OUTER_VAR)
- if it is a "before" - add to targetlist new var independently from
rel->attr_needed[attno].
Additionally I've change build_joinrel_tlist to ignore any "BEFORE"
RTEs. The regression tests are updated.
Regards,
Karol

> -----Original Message-----
> From: Tatsuo Ishii [mailto:ishii(at)postgresql(dot)org]
>
>
> Also I don't understand why you need UTF-16 support as a database
encoding
> because UTF-8 and UTF-16 are logically equivalent, they are just
different
> represention (encoding) of Unicode. That means if we already support
UTF-8
> (I'm sure we already do), there's no particular reason we need to add
UTF-16
> support.
>
> Maybe you just want to support UTF-16 as a client encoding?

Given below is a design draft for this functionality:

Core new functionality (new code):
1)Create and register independent NCHAR/NVARCHAR/NTEXT data types.

2)Provide support for the new GUC nchar_collation to provide the
database with information about the default collation that needs to be
used for the new data types.

3)Create encoding conversion subroutines to convert strings between the
database encoding and UTF8 (from national strings to regular strings and
back).
PostgreSQL already have all required support (used for conversion
between the database encoding and client_encoding), so amount of the new
code will be minimal there.

4)Because all symbols from non-UTF8 encodings could be represented as
UTF8 (but the reverse is not true) comparison between N* types and the
regular string types inside database will be performed in UTF8 form. To
achieve this feature the new IMPLICIT casts may need to be created:
NCHAR -> CHAR
NVARCHAR -> VARCHAR
NTEXT -> TEXT.

Casting in the reverse direction will be available too but only as
EXPLICIT.
However, these casts could fail if national strings could not be
represented in the used database encoding.

All these casts will use subroutines created in 3).

Casting/conversion between N* types will follow the same rules/mechanics
as used for casting/conversion between usual (CHAR(N)/VARCHAR(N)/TEXT)
string types.

5)Comparison between NATIONAL string values will be performed via
specialized UTF8 optimized functions (with respect of the
nchar_collation setting).

6)Client input/output of NATIONAL strings - NATIONAL strings will
respect the client_encoding setting, and their values will be
transparently converted to the requested client_encoding before
sending(receiving) to client (the same mechanics as used for usual
string types).
So no mixed encoding in client input/output will be supported/available.

7)Create set of the regression tests for these new data types.

Additional changes:
1)ECPG support for these new types
2) Support in the database drivers for the data types.

Rgds,
Arul Shaji

> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese: http://www.sraoss.co.jp

"Arulappan, Arul Shaji" <arul(at)fast(dot)au(dot)fujitsu(dot)com> writes:
> Given below is a design draft for this functionality:

> Core new functionality (new code):
> 1)Create and register independent NCHAR/NVARCHAR/NTEXT data types.

> 2)Provide support for the new GUC nchar_collation to provide the
> database with information about the default collation that needs to be
> used for the new data types.

A GUC seems like completely the wrong tack to be taking. In the first
place, that would mandate just one value (at a time anyway) of
collation, which is surely not much of an advance over what's already
possible. In the second place, what happens if you change the value?
All your indexes on nchar columns are corrupt, that's what. Actually
the data itself would be corrupt, if you intend that this setting
determines the encoding and not just the collation. If you really are
speaking only of collation, it's not clear to me exactly what this
proposal offers that can't be achieved today (with greater security,
functionality and spec compliance) by using COLLATE clauses on plain
text columns.

Actually, you really haven't answered at all what it is you want to do
that COLLATE can't do.

> 4)Because all symbols from non-UTF8 encodings could be represented as
> UTF8 (but the reverse is not true) comparison between N* types and the
> regular string types inside database will be performed in UTF8 form.

I believe that in some Far Eastern character sets there are some
characters that map to the same Unicode glyph, but that some people
would prefer to keep separate. So transcoding to UTF8 isn't necessarily
lossless. This is one of the reasons why we've resisted adopting ICU or
standardizing on UTF8 as the One True Database Encoding. Now this may
or may not matter for comparison to strings that were in some other
encoding to start with --- but as soon as you base your design on the
premise that UTF8 is a universal encoding, you are sliding down a
slippery slope to a design that will meet resistance.

> 6)Client input/output of NATIONAL strings - NATIONAL strings will
> respect the client_encoding setting, and their values will be
> transparently converted to the requested client_encoding before
> sending(receiving) to client (the same mechanics as used for usual
> string types).
> So no mixed encoding in client input/output will be supported/available.

If you have this restriction, then I'm really failing to see what
benefit there is over what can be done today with COLLATE.

regards, tom lane

Hi everyone,

I will try answer on all questions related to proposed National
Characters support.

>> 2)Provide support for the new GUC nchar_collation to provide the
>> database with information about the default collation that needs to
be
>> used for the new data types.

>A GUC seems like completely the wrong tack to be taking. In the first
place, that would mandate just one value (at a time anyway) of
collation, which is surely not much of an advance over what's already
possible. In the second place, what happens if you change the value?
>All your indexes on nchar columns are corrupt, that's what. Actually
the data itself would be corrupt, if you intend that this setting
determines the encoding and not just the collation. If you really are
speaking only of collation, it's not clear to me exactly what this
proposal offers that can't be achieved today (with greater security,
>functionality and spec compliance) by using COLLATE clauses on plain
text columns.
>Actually, you really haven't answered at all what it is you want to do
that COLLATE can't do.

I think I give a wrong description there... it will be not GUC but
GUC-type value which will be initialized during CREATE DATABASE and will
be read only after, very similar to the lc_collate.
So I think name national_lc_collate will be better.
Function of this value - provide information about the default collation
for the NATIONAL CHARACTERS inside the database.
That's not limits user ability of use an alternative collation for
NATIONAL CHARACTERS during create table via COLLATE keyword.

E.g. if we have second encoding inside the database - we should have
information about used collation somewhere.

>> 4)Because all symbols from non-UTF8 encodings could be represented as
>> UTF8 (but the reverse is not true) comparison between N* types and
the
>> regular string types inside database will be performed in UTF8 form.

>I believe that in some Far Eastern character sets there are some
characters that map to the same Unicode glyph, but that some people
would prefer to keep separate. So transcoding to UTF8 isn't necessarily
lossless. This is one of the reasons why we've resisted adopting ICU or
standardizing on UTF8 as the One True Database Encoding. >Now this may
or may not matter for comparison to strings that were in some other
encoding to start with --- but as soon as you base your design on the
premise that UTF8 is a universal encoding, you are sliding down a
slippery slope to a design that will meet resistance.

Will the conversion of both sides to the pg_wchar before comparison fix
this problem?
Anyway, if the database going to use more than one encoding, a some
universal encoding should be used to allow comparison between them.
After some analyse I think pg_wchar is better candidate to this role
than UTF8.

>> 6)Client input/output of NATIONAL strings - NATIONAL strings will
>> respect the client_encoding setting, and their values will be
>> transparently converted to the requested client_encoding before
>> sending(receiving) to client (the same mechanics as used for usual
>> string types).
>> So no mixed encoding in client input/output will be
supported/available.

>If you have this restriction, then I'm really failing to see what
benefit there is over what can be done today with COLLATE.

There are two targets for this project:

1. Legacy database with non-utf8 encoding, which should support old
non-utf8 applications and new UTF8 applications.
In that case the old applications will use the legacy database encoding
(and because these applications are legacy they doesn't work with new
NATIONAL CHARACTERS data/tables).
And the new applications will use client-side UTF8 encoding and they
will be able store international texts in NATIONAL CHARACTER columns.
Dump/restore of the whole database to change the database encoding to
UTF8 not always possible, so there necessity of the some easy to use
workaround.

2.Better compatibility with the ANSI SQL standard.

Kind Regards,
Maksym

Boguk, Maksym escribió:

> I think I give a wrong description there... it will be not GUC but
> GUC-type value which will be initialized during CREATE DATABASE and will
> be read only after, very similar to the lc_collate.
> So I think name national_lc_collate will be better.
> Function of this value - provide information about the default collation
> for the NATIONAL CHARACTERS inside the database.
> That's not limits user ability of use an alternative collation for
> NATIONAL CHARACTERS during create table via COLLATE keyword.

This seems a bit odd. I mean, if I want the option for differing
encodings, surely I need to be able to set them for each column, not at
the database level.

Also, as far as I understand what we want to control here is the
encoding that the strings are in (the mapping of bytes to characters),
not the collation (the way a set of strings are ordered). So it doesn't
make sense to set the NATIONAL CHARACTER option using the COLLATE
keyword.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Also, as far as I understand what we want to control here is the
> encoding that the strings are in (the mapping of bytes to characters),
> not the collation (the way a set of strings are ordered). So it doesn't
> make sense to set the NATIONAL CHARACTER option using the COLLATE
> keyword.

My thought is that we should simply ignore the NATIONAL CHARACTER syntax,
which is not the first nor the last brain-damaged feature design in the
SQL standard. It's basically useless for what we want because there's
noplace to specify which encoding you mean. Instead, let's consider that
COLLATE can define not only the collation but also the encoding of a
string datum. Contrary to what I think you meant above, that seems
perfectly sensible to me, because after all a collation is necessarily a
bunch of rules about how to order a particular set of characters. If the
data representation you use is unable to represent that set of characters,
it's not a very meaningful combination is it?

There's still the problem of how do you get a string of a nondefault
encoding into the database in the first place. If you have to convert
to DB encoding to get it in there, then what's the use of a further
conversion? This consideration may well kill the whole concept.
(It certainly kills NATIONAL CHARACTER syntax just as much.)

regards, tom lane

> From: Alvaro Herrera [mailto:alvherre(at)2ndquadrant(dot)com]
>
> Boguk, Maksym escribió:
>
> > I think I give a wrong description there... it will be not GUC but
> > GUC-type value which will be initialized during CREATE DATABASE and
> > will be read only after, very similar to the lc_collate.
> > So I think name national_lc_collate will be better.
> > Function of this value - provide information about the default
> > collation for the NATIONAL CHARACTERS inside the database.
> > That's not limits user ability of use an alternative collation for
> > NATIONAL CHARACTERS during create table via COLLATE keyword.
>
> This seems a bit odd. I mean, if I want the option for differing encodings,
> surely I need to be able to set them for each column, not at the database
> level.
>
> Also, as far as I understand what we want to control here is the encoding that
> the strings are in (the mapping of bytes to characters), not the collation

Yes, that is our idea too. For the sql syntax

Create table tbl1 (col1 nchar);

What should be the encoding and collation for col1? Because the idea is to have them in separate encoding and collation (if needed) from that of the rest of the table. We have options of

a) Having guc variables that will determine the default encoding and collation for nchar/nvarchar columns. Note that the collate variable is default only. Users can still override them per column.
b) Having the encoding name and collation as part of the syntax. For ex., (col1 nchar encoding UTF-8 COLLATE "C"). Ugly, but.....
c) Be rigid and say nchar/nvarchar columns are by default UTF-8 (or something else). One cannot change the default. But they can override it when declaring the column by having a syntax similar to (b)

Rgds,
Arul Shaji

> (the way a set of strings are ordered). So it doesn't make sense to set the
> NATIONAL CHARACTER option using the COLLATE keyword.

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

> From: Tom Lane [mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us]
>
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Also, as far as I understand what we want to control here is the
> > encoding that the strings are in (the mapping of bytes to
characters),
> > not the collation (the way a set of strings are ordered). So it
> > doesn't make sense to set the NATIONAL CHARACTER option using the
> > COLLATE keyword.
>
> My thought is that we should simply ignore the NATIONAL CHARACTER
syntax,
> which is not the first nor the last brain-damaged feature design in
the SQL
> standard. It's basically useless for what we want because there's
noplace to
> specify which encoding you mean. Instead, let's consider that COLLATE
can
> define not only the collation but also the encoding of a string datum.

Yes, don't have a problem with this. If I understand you correctly, this
will be simpler syntax wise, but still get nchar/nvarchar data types
into a table, in different encoding from the rest of the table.

>
> There's still the problem of how do you get a string of a nondefault
encoding
> into the database in the first place.

Yes, that is the bulk of the work. Will need change in a whole lot of
places.

Is a step-by-step approach worth exploring ? Something similar to:

Step 1: Support nchar/nvarchar data types. Restrict them only to UTF-8
databases to begin with.
Step 2: Support multiple encodings in a database. Remove the restriction
imposed in step1.

Rgds,
Arul Shaji

Hi,

mini-review follows.

2013-07-22 21:52 keltezéssel, Karol Trzcionka írta:
> I've noticed problem with "UPDATE ... FROM" statement. Fix in new version.
> Regards,
> Karol

* Does it apply cleanly to the current git master?

No. There's a reject in src/backend/optimizer/plan/initsplan.c

* Does it include reasonable tests?

Yes but the test fails after trying to fix the rejected chunk of the patch.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
> * Does it apply cleanly to the current git master?
>
> No. There's a reject in src/backend/optimizer/plan/initsplan.c
Thank you, merged in attached version.
>
> * Does it include reasonable tests?
>
> Yes but the test fails after trying to fix the rejected chunk of the
> patch.
It fails because the "HINT" was changed, fixed.
That version merges some nested "ifs" left over from earlier work.
Regards,
Karol Trzcionka

2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
> W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
>> * Does it apply cleanly to the current git master?
>>
>> No. There's a reject in src/backend/optimizer/plan/initsplan.c
> Thank you, merged in attached version.
>> * Does it include reasonable tests?
>>
>> Yes but the test fails after trying to fix the rejected chunk of the
>> patch.
> It fails because the "HINT" was changed, fixed.
> That version merges some nested "ifs" left over from earlier work.

I tried to compile your v5 patch and I got:

initsplan.c: In function ‘add_vars_to_targetlist’:
initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
rel->reltargetlist = lappend(rel->reltargetlist,
^

You shouldn't change the assignment at declaration:

- RelOptInfo *rel = find_base_rel(root, var->varno);
+ RelOptInfo *rel;
...
+ if (root->parse->commandType == CMD_UPDATE)
+ {
... (code using rel)
+ }
+ rel = find_base_rel(root, varno);

Best regards,
Zoltán Böszörményi

> Regards,
> Karol Trzcionka

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Hi,

2013-08-19 21:52 keltezéssel, Boszormenyi Zoltan írta:
> 2013-08-19 21:21 keltezéssel, Karol Trzcionka írta:
>> W dniu 19.08.2013 19:56, Boszormenyi Zoltan pisze:
>>> * Does it apply cleanly to the current git master?
>>>
>>> No. There's a reject in src/backend/optimizer/plan/initsplan.c
>> Thank you, merged in attached version.
>>> * Does it include reasonable tests?
>>>
>>> Yes but the test fails after trying to fix the rejected chunk of the
>>> patch.
>> It fails because the "HINT" was changed, fixed.
>> That version merges some nested "ifs" left over from earlier work.
>
> I tried to compile your v5 patch and I got:
>
> initsplan.c: In function ‘add_vars_to_targetlist’:
> initsplan.c:216:26: warning: ‘rel’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> rel->reltargetlist = lappend(rel->reltargetlist,
> ^
>
> You shouldn't change the assignment at declaration:
>
> - RelOptInfo *rel = find_base_rel(root, var->varno);
> + RelOptInfo *rel;
> ...
> + if (root->parse->commandType == CMD_UPDATE)
> + {
> ... (code using rel)
> + }
> + rel = find_base_rel(root, varno);

Let me say it again: the new code in initsplan.c::add_vars_to_targetlist() is fishy.
The compiler says that "rel" used on line 216 may be uninitialized.

Keeping it that way passes "make check", perhaps "rel" was initialized
in a previous iteration of "foreach(temp, vars)", possibly in the
else if (IsA(node, PlaceHolderVar))
branch, which means that "PlaceHolderInfo *phinfo" may be interpreted
as RelOptInfo *, stomping on memory.

Moving the assignment back to the declaration makes "make check"
fail with the attached regression.diffs file.

Initializing it as "RelOptInfo *rel = NULL;" makes the regression check
die with a segfault, obviously.

Change the code to avoid the warning and still produce the wanted effect.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Thank you for the review and tests. New version introduce a lot of
improvements:
- Fix regression test for view (wrong table_name)
- Add regression test for inheritance
- Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
uninitialized issue
- Revert changing varno in add_vars_to_targetlist
- Add all "before" variables to targetlist
- Avoid adding variables to slot for AFTER.
- Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
- All before/after are now set on OUTER_VAR
- Rename fix_varno_varattno to bind_returning_variables
- Add comment about bind_returning_variables
- Remove unneeded code in fix_join_expr_mutator (it was changing varno
of RTE_BEFORE - now there is not any var with varno assigned to it)
Regards,
Karol Trzcionka

W dniu 20.08.2013 16:47, Karol Trzcionka pisze:
> Thank you for the review and tests. New version introduce a lot of
> improvements:
> - Fix regression test for view (wrong table_name)
> - Add regression test for inheritance
> - Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
> uninitialized issue
> - Revert changing varno in add_vars_to_targetlist
> - Add all "before" variables to targetlist
> - Avoid adding variables to slot for AFTER.
> - Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
> - All before/after are now set on OUTER_VAR
> - Rename fix_varno_varattno to bind_returning_variables
> - Add comment about bind_returning_variables
> - Remove unneeded code in fix_join_expr_mutator (it was changing varno
> of RTE_BEFORE - now there is not any var with varno assigned to it)
I've just realized the prepare_returning_before() is unneeded right now
so I've removed it. Version 7, hopefully the last. ;)
Regards,
Karol Trzcionka

2013-08-20 17:30 keltezéssel, Karol Trzcionka írta:
> W dniu 20.08.2013 16:47, Karol Trzcionka pisze:
>> Thank you for the review and tests. New version introduce a lot of
>> improvements:
>> - Fix regression test for view (wrong table_name)
>> - Add regression test for inheritance
>> - Delete hack in initsplan.c (now we ignore all RTE_BEFORE) - the
>> uninitialized issue
>> - Revert changing varno in add_vars_to_targetlist
>> - Add all "before" variables to targetlist
>> - Avoid adding variables to slot for AFTER.
>> - Treat varnoold like a flag - prevent from adjustment if RTE_BEFORE
>> - All before/after are now set on OUTER_VAR
>> - Rename fix_varno_varattno to bind_returning_variables
>> - Add comment about bind_returning_variables
>> - Remove unneeded code in fix_join_expr_mutator (it was changing varno
>> of RTE_BEFORE - now there is not any var with varno assigned to it)
> I've just realized the prepare_returning_before() is unneeded right now
> so I've removed it. Version 7, hopefully the last. ;)

Here's a new one, for v7:

setrefs.c: In function ‘set_plan_refs’:
setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
bind_returning_variables(rlist, before_index, after_index);
^
setrefs.c:1957:21: note: ‘before_index’ was declared here
int after_index=0, before_index;
^

Best regards,
Zoltán Böszörményi

> Regards,
> Karol Trzcionka
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
> Here's a new one, for v7:
>
> setrefs.c: In function ‘set_plan_refs’:
> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized
> in this function [-Wmaybe-uninitialized]
> bind_returning_variables(rlist, before_index, after_index);
> ^
> setrefs.c:1957:21: note: ‘before_index’ was declared here
> int after_index=0, before_index;
> ^
Right, my mistake. Sorry and thanks. Fixed.
Regards,
Karol Trzcionka

Hi,

2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
> W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
>> Here's a new one, for v7:
>>
>> setrefs.c: In function ‘set_plan_refs’:
>> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
>> [-Wmaybe-uninitialized]
>> bind_returning_variables(rlist, before_index, after_index);
>> ^
>> setrefs.c:1957:21: note: ‘before_index’ was declared here
>> int after_index=0, before_index;
>> ^
> Right, my mistake. Sorry and thanks. Fixed.
> Regards,
> Karol Trzcionka

With this fixed, a more complete review:

* Is the patch in a patch format which has context? (eg: context diff format)

Yes.

* Does it apply cleanly to the current git master?

Yes.

* Does it include reasonable tests, necessary doc patches, etc?

There is a new regression test (returning_before_after.sql) covering
this feature. However, I think it should be added to the group
where "returning.sql" resides currently. There is a value in running it
in parallel to other tests. Sometimes a subtle bug is uncovered
because of this and your v2 patch fixed such a bug already.

doc/src/sgml/ref/update.sgml describes this feature.

doc/src/sgml/dml.sgml should also be touched to cover this feature.

* Does the patch actually implement what it's supposed to do?

Yes.

* Do we want that?

Yes.

* Do we already have it?

No.

* Does it follow SQL spec, or the community-agreed behavior?

RETURNING is a PostgreSQL extension, so the SQL-spec part
of the question isn't applicable.

It implements the community-agreed behaviour, according to
the new regression test coverage.

* Does it include pg_dump support (if applicable)?

n/a

* Are there dangers?

I don't think so.

* Have all the bases been covered?

It seems so. I have also tried mixing before/after columns in
different orders and the query didn't fail:

zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text);
CREATE TABLE
zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
INSERT 0 1
zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
INSERT 0 1
zozo=# select * from t1;
id | i1 | i2 | t1 | t2
----+----+----+----+----
1 | 1 | 1 | a | a
2 | 2 | 2 | b | b
3 | 3 | 3 | c | c
(3 rows)

zozo=# begin;
BEGIN
zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
----+----+----+----+----+----+----+-----
2 | 2 | 2 | 4 | b | b | b | bx2
(1 row)

UPDATE 1
zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id = 3
returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
----+----+----+----+----+----+-----+-----
3 | 3 | 9 | 6 | c | c | cx3 | cx2
(1 row)

UPDATE 1

* Does the feature work as advertised?

Yes.

* Are there corner cases the author has failed to consider?

I don't know.

* Are there any assertion failures or crashes?

No.

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

n/a

* Does it slow down other things?

No.

* Does it follow the project coding guidelines?

Mostly.

In the src/test/regress/parallel_schedule contains an extra
new line at the end, it shouldn't.

In b/src/backend/optimizer/plan/setrefs.c:

+static void bind_returning_variables(List *rlist, int bef, int aft);

but later it becomes not public:

+ */
+void bind_returning_variables(List *rlist, int bef, int aft)
+{

Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
declaration is not needed, the function is called only from
later functions.

Similar for parse_clause.c:

+extern void addAliases(ParseState *pstate);

+void addAliases(ParseState *pstate)

This external declaration is not needed since it is already
in src/include/parser/parse_clause.h

In setrefs.c, bind_returning_variables() I would also rename
the function arguments, so "before" and "after" are spelled out.
These are not C keywords.

Some assignments, like:

+ var=(Var*)tle;
and
+ int index_rel=1;

in setrefs.c need spaces.

"if()" statements need a space before the "(" and not after.

Add spaces in the {} list in addAliases():
+ char *aliases[] = {"before","after"};
like this: { "before", "after" }

Spaces are needed here, too:
+ for(i=0 ; i<noal; i++)

This "noal" should be "naliases" or "n_aliases" if you really want
a variable. I would simply use the constant "2" for the two for()
loops in addAliases() instead, its purpose is obvious enough.

In setrefs.c, bind_returning_variables():
+ Var *var = NULL;
+ foreach(temp, rlist){
Add an empty line after the declaration block.

* Are there portability issues?

No.

* Will it work on Windows/BSD etc?

Yes.

* Are the comments sufficient and accurate?

* Does it do what it says, correctly?

Yes.

* Does it produce compiler warnings?

No.

* Can you make it crash?

No.

* Is everything done in a way that fits together coherently with other features/modules?

I think so, mostly. Comments below.

* Are there interdependencies that can cause problems?

I don't think so.

Other comments:

This #define in pg_class:

diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 49c4f6f..1b09994 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -154,6 +154,7 @@ DESCR("");
#define RELKIND_COMPOSITE_TYPE 'c' /* composite type */
#define RELKIND_FOREIGN_TABLE 'f' /* foreign table */
#define RELKIND_MATVIEW 'm' /* materialized view */
+#define RELKIND_BEFORE 'b' /* virtual table for before/after
statements */

#define RELPERSISTENCE_PERMANENT 'p' /* regular table */
#define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
table */

The "RELKIND_*" values all show up in the pg_class table except
this new one. I don't think pg_class.h should be modified at all.
addAliases() should use RELKIND_RELATION together with
RTE_BEFORE. Then checks like:

+ if (rte->relkind == RELKIND_BEFORE)
+ continue;

should become

+ if (rte->relkind == RELKIND_RELATION && rte->rtekind == RTE_BEFORE)
+ continue;

Speaking of which, RTE_BEFORE is more properly named
RTE_RETURNING_ALIAS or something like that because it
covers both "before" and "after". Someone may have a better
idea for naming this symbol.

I feel like I understand what the code does and it looks sane to me.

One question, though, about this part:

----------------------------------------
@@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
int rtoffset)
{
indexed_tlist *itlist;
+ int after_index=0, before_index=0;
+ Query *parse = root->parse;

+ ListCell *rt;
+ RangeTblEntry *bef;
+
+ int index_rel=1;
+
+ foreach(rt,parse->rtable)
+ {
+ bef = (RangeTblEntry *)lfirst(rt);
+ if(strcmp(bef->eref->aliasname,"after") == 0 && bef->rtekind == RTE_BEFORE )
+ {
+ after_index = index_rel;
+ }
+ if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind == RTE_BEFORE )
+ {
+ before_index = index_rel;
+ }
+ index_rel++;
+ }
/*
* We can perform the desired Var fixup by abusing the fix_join_expr
* machinery that formerly handled inner indexscan fixup. We search the
@@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
resultRelation,
rtoffset);

+ bind_returning_variables(rlist, before_index, after_index);
pfree(itlist);

return rlist;
----------------------------------------

Why is it enough to keep the last before_index and after_index values?
What if there are more than one matching RangeTblEntry for "before"
and/or for "after"? Is it an error condition or of them should be fixed?

I think that's all for now.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:
> Hi,
>
> 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
>> W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
>>> Here's a new one, for v7:
>>>
>>> setrefs.c: In function ‘set_plan_refs’:
>>> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
>>> [-Wmaybe-uninitialized]
>>> bind_returning_variables(rlist, before_index, after_index);
>>> ^
>>> setrefs.c:1957:21: note: ‘before_index’ was declared here
>>> int after_index=0, before_index;
>>> ^
>> Right, my mistake. Sorry and thanks. Fixed.
>> Regards,
>> Karol Trzcionka
>
> With this fixed, a more complete review:
>
> * Is the patch in a patch format which has context? (eg: context diff format)
>
> Yes.
>
> * Does it apply cleanly to the current git master?
>
> Yes.
>
> * Does it include reasonable tests, necessary doc patches, etc?
>
> There is a new regression test (returning_before_after.sql) covering
> this feature. However, I think it should be added to the group
> where "returning.sql" resides currently. There is a value in running it
> in parallel to other tests. Sometimes a subtle bug is uncovered
> because of this and your v2 patch fixed such a bug already.
>
> doc/src/sgml/ref/update.sgml describes this feature.
>
> doc/src/sgml/dml.sgml should also be touched to cover this feature.
>
> * Does the patch actually implement what it's supposed to do?
>
> Yes.
>
> * Do we want that?
>
> Yes.
>
> * Do we already have it?
>
> No.
>
> * Does it follow SQL spec, or the community-agreed behavior?
>
> RETURNING is a PostgreSQL extension, so the SQL-spec part
> of the question isn't applicable.
>
> It implements the community-agreed behaviour, according to
> the new regression test coverage.
>
> * Does it include pg_dump support (if applicable)?
>
> n/a
>
> * Are there dangers?
>
> I don't think so.
>
> * Have all the bases been covered?
>
> It seems so. I have also tried mixing before/after columns in
> different orders and the query didn't fail:
>
> zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text);
> CREATE TABLE
> zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
> INSERT 0 1
> zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
> INSERT 0 1
> zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
> INSERT 0 1
> zozo=# select * from t1;
> id | i1 | i2 | t1 | t2
> ----+----+----+----+----
> 1 | 1 | 1 | a | a
> 2 | 2 | 2 | b | b
> 3 | 3 | 3 | c | c
> (3 rows)
>
> zozo=# begin;
> BEGIN
> zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
> after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
> i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
> ----+----+----+----+----+----+----+-----
> 2 | 2 | 2 | 4 | b | b | b | bx2
> (1 row)
>
> UPDATE 1
> zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id =
> 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
> after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
> ----+----+----+----+----+----+-----+-----
> 3 | 3 | 9 | 6 | c | c | cx3 | cx2
> (1 row)
>
> UPDATE 1
>
>
>
> * Does the feature work as advertised?
>
> Yes.
>
> * Are there corner cases the author has failed to consider?
>
> I don't know.
>
> * Are there any assertion failures or crashes?
>
> No.
>
> * Does the patch slow down simple tests?
>
> No.
>
> * If it claims to improve performance, does it?
>
> n/a
>
> * Does it slow down other things?
>
> No.
>
> * Does it follow the project coding guidelines?
>
> Mostly.
>
> In the src/test/regress/parallel_schedule contains an extra
> new line at the end, it shouldn't.
>
> In b/src/backend/optimizer/plan/setrefs.c:
>
> +static void bind_returning_variables(List *rlist, int bef, int aft);
>
> but later it becomes not public:
>
> + */
> +void bind_returning_variables(List *rlist, int bef, int aft)
> +{
>
> Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
> declaration is not needed, the function is called only from
> later functions.
>
> Similar for parse_clause.c:
>
> +extern void addAliases(ParseState *pstate);
>
> +void addAliases(ParseState *pstate)
>
> This external declaration is not needed since it is already
> in src/include/parser/parse_clause.h
>
> In setrefs.c, bind_returning_variables() I would also rename
> the function arguments, so "before" and "after" are spelled out.
> These are not C keywords.
>
> Some assignments, like:
>
> + var=(Var*)tle;
> and
> + int index_rel=1;
>
> in setrefs.c need spaces.
>
> "if()" statements need a space before the "(" and not after.
>
> Add spaces in the {} list in addAliases():
> + char *aliases[] = {"before","after"};
> like this: { "before", "after" }
>
> Spaces are needed here, too:
> + for(i=0 ; i<noal; i++)
>
> This "noal" should be "naliases" or "n_aliases" if you really want
> a variable. I would simply use the constant "2" for the two for()
> loops in addAliases() instead, its purpose is obvious enough.
>
> In setrefs.c, bind_returning_variables():
> + Var *var = NULL;
> + foreach(temp, rlist){
> Add an empty line after the declaration block.
>
>
> * Are there portability issues?
>
> No.
>
> * Will it work on Windows/BSD etc?
>
> Yes.
>
> * Are the comments sufficient and accurate?

There should be more comments, especially regarding
my question at the end.

>
>
>
> * Does it do what it says, correctly?
>
> Yes.
>
> * Does it produce compiler warnings?
>
> No.
>
> * Can you make it crash?
>
> No.
>
> * Is everything done in a way that fits together coherently with other features/modules?
>
> I think so, mostly. Comments below.
>
> * Are there interdependencies that can cause problems?
>
> I don't think so.
>
> Other comments:
>
> This #define in pg_class:
>
> diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
> index 49c4f6f..1b09994 100644
> --- a/src/include/catalog/pg_class.h
> +++ b/src/include/catalog/pg_class.h
> @@ -154,6 +154,7 @@ DESCR("");
> #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */
> #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */
> #define RELKIND_MATVIEW 'm' /* materialized view */
> +#define RELKIND_BEFORE 'b' /* virtual table for before/after
> statements */
>
> #define RELPERSISTENCE_PERMANENT 'p' /* regular table */
> #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
> table */
>
> The "RELKIND_*" values all show up in the pg_class table except
> this new one. I don't think pg_class.h should be modified at all.
> addAliases() should use RELKIND_RELATION together with
> RTE_BEFORE. Then checks like:
>
> + if (rte->relkind == RELKIND_BEFORE)
> + continue;
>
> should become
>
> + if (rte->relkind == RELKIND_RELATION && rte->rtekind == RTE_BEFORE)
> + continue;

Thinking about it more,
if (rte->rtekind == RTE_BEFORE)
would be enough, as no other kinds of rte's can have rtekind == RTE_BEFORE.

>
> Speaking of which, RTE_BEFORE is more properly named
> RTE_RETURNING_ALIAS or something like that because it
> covers both "before" and "after". Someone may have a better
> idea for naming this symbol.
>
> I feel like I understand what the code does and it looks sane to me.
>
> One question, though, about this part:
>
> ----------------------------------------
> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
> int rtoffset)
> {
> indexed_tlist *itlist;
> + int after_index=0, before_index=0;
> + Query *parse = root->parse;
>
> + ListCell *rt;
> + RangeTblEntry *bef;
> +
> + int index_rel=1;
> +
> + foreach(rt,parse->rtable)
> + {
> + bef = (RangeTblEntry *)lfirst(rt);
> + if(strcmp(bef->eref->aliasname,"after") == 0 && bef->rtekind == RTE_BEFORE )
> + {
> + after_index = index_rel;
> + }
> + if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind ==
> RTE_BEFORE )
> + {
> + before_index = index_rel;
> + }
> + index_rel++;
> + }
> /*
> * We can perform the desired Var fixup by abusing the fix_join_expr
> * machinery that formerly handled inner indexscan fixup. We search the
> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
> resultRelation,
> rtoffset);
>
> + bind_returning_variables(rlist, before_index, after_index);
> pfree(itlist);
>
> return rlist;
> ----------------------------------------
>
> Why is it enough to keep the last before_index and after_index values?
> What if there are more than one matching RangeTblEntry for "before"
> and/or for "after"? Is it an error condition or of them should be fixed?
>
> I think that's all for now.
>
> Best regards,
> Zoltán Böszörményi
>
> --
> ----------------------------------
> Zoltán Böszörményi
> Cybertec Schönig & Schönig GmbH
> Gröhrmühlgasse 26
> A-2700 Wiener Neustadt, Austria
> Web:http://www.postgresql-support.de
> http://www.postgresql.at/

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

2013-08-21 20:00 keltezéssel, Boszormenyi Zoltan írta:
> 2013-08-21 19:17 keltezéssel, Boszormenyi Zoltan írta:
>> Hi,
>>
>> 2013-08-20 21:06 keltezéssel, Karol Trzcionka írta:
>>> W dniu 20.08.2013 20:55, Boszormenyi Zoltan pisze:
>>>> Here's a new one, for v7:
>>>>
>>>> setrefs.c: In function ‘set_plan_refs’:
>>>> setrefs.c:2001:26: warning: ‘before_index’ may be used uninitialized in this function
>>>> [-Wmaybe-uninitialized]
>>>> bind_returning_variables(rlist, before_index, after_index);
>>>> ^
>>>> setrefs.c:1957:21: note: ‘before_index’ was declared here
>>>> int after_index=0, before_index;
>>>> ^
>>> Right, my mistake. Sorry and thanks. Fixed.
>>> Regards,
>>> Karol Trzcionka
>>
>> With this fixed, a more complete review:
>>
>> * Is the patch in a patch format which has context? (eg: context diff format)
>>
>> Yes.
>>
>> * Does it apply cleanly to the current git master?
>>
>> Yes.
>>
>> * Does it include reasonable tests, necessary doc patches, etc?
>>
>> There is a new regression test (returning_before_after.sql) covering
>> this feature. However, I think it should be added to the group
>> where "returning.sql" resides currently. There is a value in running it
>> in parallel to other tests. Sometimes a subtle bug is uncovered
>> because of this and your v2 patch fixed such a bug already.
>>
>> doc/src/sgml/ref/update.sgml describes this feature.
>>
>> doc/src/sgml/dml.sgml should also be touched to cover this feature.
>>
>> * Does the patch actually implement what it's supposed to do?
>>
>> Yes.
>>
>> * Do we want that?
>>
>> Yes.
>>
>> * Do we already have it?
>>
>> No.
>>
>> * Does it follow SQL spec, or the community-agreed behavior?
>>
>> RETURNING is a PostgreSQL extension, so the SQL-spec part
>> of the question isn't applicable.
>>
>> It implements the community-agreed behaviour, according to
>> the new regression test coverage.
>>
>> * Does it include pg_dump support (if applicable)?
>>
>> n/a
>>
>> * Are there dangers?
>>
>> I don't think so.
>>
>> * Have all the bases been covered?
>>
>> It seems so. I have also tried mixing before/after columns in
>> different orders and the query didn't fail:
>>
>> zozo=# create table t1 (id serial primary key, i1 int4, i2 int4, t1 text, t2 text);
>> CREATE TABLE
>> zozo=# insert into t1 (i1, i2, t1, t2) values (1, 1, 'a', 'a');
>> INSERT 0 1
>> zozo=# insert into t1 (i1, i2, t1, t2) values (2, 2, 'b', 'b');
>> INSERT 0 1
>> zozo=# insert into t1 (i1, i2, t1, t2) values (3, 3, 'c', 'c');
>> INSERT 0 1
>> zozo=# select * from t1;
>> id | i1 | i2 | t1 | t2
>> ----+----+----+----+----
>> 1 | 1 | 1 | a | a
>> 2 | 2 | 2 | b | b
>> 3 | 3 | 3 | c | c
>> (3 rows)
>>
>> zozo=# begin;
>> BEGIN
>> zozo=# update t1 set i2 = i2*2, t2 = t2 || 'x2' where id = 2 returning before.i1,
>> after.i1, before.i2, after.i2, before.t1, after.t1, before.t2, after.t2;
>> i1 | i1 | i2 | i2 | t1 | t1 | t2 | t2
>> ----+----+----+----+----+----+----+-----
>> 2 | 2 | 2 | 4 | b | b | b | bx2
>> (1 row)
>>
>> UPDATE 1
>> zozo=# update t1 set i1 = i1 * 3, i2 = i2*2, t1 = t1 || 'x3', t2 = t2 || 'x2' where id
>> = 3 returning before.i1, before.i2, after.i1, after.i2, before.t1, before.t2, after.t1,
>> after.t2; i1 | i2 | i1 | i2 | t1 | t2 | t1 | t2
>> ----+----+----+----+----+----+-----+-----
>> 3 | 3 | 9 | 6 | c | c | cx3 | cx2
>> (1 row)
>>
>> UPDATE 1
>>
>>
>>
>> * Does the feature work as advertised?
>>
>> Yes.
>>
>> * Are there corner cases the author has failed to consider?
>>
>> I don't know.
>>
>> * Are there any assertion failures or crashes?
>>
>> No.
>>
>> * Does the patch slow down simple tests?
>>
>> No.
>>
>> * If it claims to improve performance, does it?
>>
>> n/a
>>
>> * Does it slow down other things?
>>
>> No.
>>
>> * Does it follow the project coding guidelines?
>>
>> Mostly.
>>
>> In the src/test/regress/parallel_schedule contains an extra
>> new line at the end, it shouldn't.
>>
>> In b/src/backend/optimizer/plan/setrefs.c:
>>
>> +static void bind_returning_variables(List *rlist, int bef, int aft);
>>
>> but later it becomes not public:
>>
>> + */
>> +void bind_returning_variables(List *rlist, int bef, int aft)
>> +{
>>
>> Strange, but GCC 4.8.1 -Wall doesn't catch it. But the forward
>> declaration is not needed, the function is called only from
>> later functions.
>>
>> Similar for parse_clause.c:
>>
>> +extern void addAliases(ParseState *pstate);
>>
>> +void addAliases(ParseState *pstate)
>>
>> This external declaration is not needed since it is already
>> in src/include/parser/parse_clause.h
>>
>> In setrefs.c, bind_returning_variables() I would also rename
>> the function arguments, so "before" and "after" are spelled out.
>> These are not C keywords.
>>
>> Some assignments, like:
>>
>> + var=(Var*)tle;
>> and
>> + int index_rel=1;
>>
>> in setrefs.c need spaces.
>>
>> "if()" statements need a space before the "(" and not after.
>>
>> Add spaces in the {} list in addAliases():
>> + char *aliases[] = {"before","after"};
>> like this: { "before", "after" }
>>
>> Spaces are needed here, too:
>> + for(i=0 ; i<noal; i++)
>>
>> This "noal" should be "naliases" or "n_aliases" if you really want
>> a variable. I would simply use the constant "2" for the two for()
>> loops in addAliases() instead, its purpose is obvious enough.
>>
>> In setrefs.c, bind_returning_variables():
>> + Var *var = NULL;
>> + foreach(temp, rlist){
>> Add an empty line after the declaration block.
>>
>>
>> * Are there portability issues?
>>
>> No.
>>
>> * Will it work on Windows/BSD etc?
>>
>> Yes.
>>
>> * Are the comments sufficient and accurate?
>
> There should be more comments, especially regarding
> my question at the end.
>
>>
>>
>>
>> * Does it do what it says, correctly?
>>
>> Yes.
>>
>> * Does it produce compiler warnings?
>>
>> No.
>>
>> * Can you make it crash?
>>
>> No.
>>
>> * Is everything done in a way that fits together coherently with other features/modules?
>>
>> I think so, mostly. Comments below.
>>
>> * Are there interdependencies that can cause problems?
>>
>> I don't think so.
>>
>> Other comments:
>>
>> This #define in pg_class:
>>
>> diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
>> index 49c4f6f..1b09994 100644
>> --- a/src/include/catalog/pg_class.h
>> +++ b/src/include/catalog/pg_class.h
>> @@ -154,6 +154,7 @@ DESCR("");
>> #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */
>> #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */
>> #define RELKIND_MATVIEW 'm' /* materialized view */
>> +#define RELKIND_BEFORE 'b' /* virtual table for
>> before/after statements */
>>
>> #define RELPERSISTENCE_PERMANENT 'p' /* regular table */
>> #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
>> table */
>>
>> The "RELKIND_*" values all show up in the pg_class table except
>> this new one. I don't think pg_class.h should be modified at all.
>> addAliases() should use RELKIND_RELATION together with
>> RTE_BEFORE. Then checks like:
>>
>> + if (rte->relkind == RELKIND_BEFORE)
>> + continue;
>>
>> should become
>>
>> + if (rte->relkind == RELKIND_RELATION && rte->rtekind == RTE_BEFORE)
>> + continue;
>
> Thinking about it more,
> if (rte->rtekind == RTE_BEFORE)
> would be enough, as no other kinds of rte's can have rtekind == RTE_BEFORE.
>
>>
>> Speaking of which, RTE_BEFORE is more properly named
>> RTE_RETURNING_ALIAS or something like that because it
>> covers both "before" and "after". Someone may have a better
>> idea for naming this symbol.
>>
>> I feel like I understand what the code does and it looks sane to me.
>>
>> One question, though, about this part:
>>
>> ----------------------------------------
>> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
>> int rtoffset)
>> {
>> indexed_tlist *itlist;
>> + int after_index=0, before_index=0;
>> + Query *parse = root->parse;
>>
>> + ListCell *rt;
>> + RangeTblEntry *bef;
>> +
>> + int index_rel=1;
>> +
>> + foreach(rt,parse->rtable)
>> + {
>> + bef = (RangeTblEntry *)lfirst(rt);
>> + if(strcmp(bef->eref->aliasname,"after") == 0 && bef->rtekind ==
>> RTE_BEFORE )
>> + {
>> + after_index = index_rel;
>> + }
>> + if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind ==
>> RTE_BEFORE )
>> + {
>> + before_index = index_rel;
>> + }
>> + index_rel++;
>> + }
>> /*
>> * We can perform the desired Var fixup by abusing the fix_join_expr
>> * machinery that formerly handled inner indexscan fixup. We search the
>> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
>> resultRelation,
>> rtoffset);
>>
>> + bind_returning_variables(rlist, before_index, after_index);
>> pfree(itlist);
>>
>> return rlist;
>> ----------------------------------------
>>
>> Why is it enough to keep the last before_index and after_index values?
>> What if there are more than one matching RangeTblEntry for "before"
>> and/or for "after"? Is it an error condition or of them should be fixed?

Since addAliases() only adds a single one for each and only for an
UPDATE ... RETURNING query, it is okay. Also, because
set_returning_clause_references() is called separately for each
query with RETURNING. Anyway, a comment before the new
foreach() loop in set_returning_clause_references() should explain
the fact that only one of each ("before" and "after") can occur for
such a query.

I have just tried this:

before as (update t1 set i1 = i1 * 2, i2 = i2 * 3, t1 = t1 || 'x2', t2 = t2 || 'x3'
where id = 1 returning before.i1, after.i1, before.i2, after.i2),
after as (update t1 set i1 = i1 * 2, i2 = i2 * 3, t1 = t1 || 'x2', t2 = t2 || 'x3'
where id = 2 returning before.i1, after.i1, before.i2, after.i2)
select * from (select * from before union all select * from after) as x;

and it gave me the proper results, no crash.

About addAliases():
- it can be moved to parser/analyze.c so it can be static.
- addReturningAliases() may be a better name for the function.

>>
>> I think that's all for now.
>>
>> Best regards,
>> Zoltán Böszörményi
>>
>> --
>> ----------------------------------
>> Zoltán Böszörményi
>> Cybertec Schönig & Schönig GmbH
>> Gröhrmühlgasse 26
>> A-2700 Wiener Neustadt, Austria
>> Web:http://www.postgresql-support.de
>> http://www.postgresql.at/
>
>
> --
> ----------------------------------
> Zoltán Böszörményi
> Cybertec Schönig & Schönig GmbH
> Gröhrmühlgasse 26
> A-2700 Wiener Neustadt, Austria
> Web:http://www.postgresql-support.de
> http://www.postgresql.at/

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> With this fixed, a more complete review:
Thanks.
> There is a new regression test (returning_before_after.sql) covering
> this feature. However, I think it should be added to the group
> where "returning.sql" resides currently. There is a value in running it
> in parallel to other tests. Sometimes a subtle bug is uncovered
> because of this and your v2 patch fixed such a bug already.
Modified to work correct in parallel testing
> doc/src/sgml/ref/update.sgml describes this feature.
>
> doc/src/sgml/dml.sgml should also be touched to cover this feature.
I don't think so, there is not any info about returning feature, I think
it shouldn't be part of my patch.
> In the src/test/regress/parallel_schedule contains an extra
> new line at the end, it shouldn't.
Fixed
>
> In b/src/backend/optimizer/plan/setrefs.c:
>
> +static void bind_returning_variables(List *rlist, int bef, int aft);
>
> but later it becomes not public:
>
> + */
> +void bind_returning_variables(List *rlist, int bef, int aft)
> +{
I've change to static in the definition.
>
> +extern void addAliases(ParseState *pstate);
>
> +void addAliases(ParseState *pstate)
>
> This external declaration is not needed since it is already
> in src/include/parser/parse_clause.h
Removed.
>
> In setrefs.c, bind_returning_variables() I would also rename
> the function arguments, so "before" and "after" are spelled out.
> These are not C keywords.
Changed.
> Some assignments, like:
>
> + var=(Var*)tle;
> and
> + int index_rel=1;
>
> in setrefs.c need spaces.
>
> "if()" statements need a space before the "(" and not after.
>
> Add spaces in the {} list in addAliases():
> + char *aliases[] = {"before","after"};
> like this: { "before", "after" }
>
> Spaces are needed here, too:
> + for(i=0 ; i<noal; i++)
>
> This "noal" should be "naliases" or "n_aliases" if you really want
> a variable. I would simply use the constant "2" for the two for()
> loops in addAliases() instead, its purpose is obvious enough.
Added some whitespaces.
> In setrefs.c, bind_returning_variables():
> + Var *var = NULL;
> + foreach(temp, rlist){
> Add an empty line after the declaration block.
Added.
> Other comments:
>
> This #define in pg_class:
>
> diff --git a/src/include/catalog/pg_class.h
> b/src/include/catalog/pg_class.h
> index 49c4f6f..1b09994 100644
> --- a/src/include/catalog/pg_class.h
> +++ b/src/include/catalog/pg_class.h
> @@ -154,6 +154,7 @@ DESCR("");
> #define RELKIND_COMPOSITE_TYPE 'c' /*
> composite type */
> #define RELKIND_FOREIGN_TABLE 'f' /*
> foreign table */
> #define RELKIND_MATVIEW
> 'm' /* materialized view */
> +#define RELKIND_BEFORE
> 'b' /* virtual table for before/after statements */
>
> #define RELPERSISTENCE_PERMANENT
> 'p' /* regular table */
> #define RELPERSISTENCE_UNLOGGED
> 'u' /* unlogged permanent table */
>
RELKIND_BEFORE removed - it probably left over previous work and/or I
needed it because RTE_BEFORE wasn't available (not included?).
> Speaking of which, RTE_BEFORE is more properly named
> RTE_RETURNING_ALIAS or something like that because it
> covers both "before" and "after". Someone may have a better
> idea for naming this symbol.
Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases)
> One question, though, about this part:
>
> ----------------------------------------
> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
> int
> rtoffset)
> {
> indexed_tlist *itlist;
> + int after_index=0, before_index=0;
> + Query *parse = root->parse;
>
> + ListCell *rt;
> + RangeTblEntry *bef;
> +
> + int index_rel=1;
> +
> + foreach(rt,parse->rtable)
> + {
> + bef = (RangeTblEntry *)lfirst(rt);
> + if(strcmp(bef->eref->aliasname,"after") == 0 &&
> bef->rtekind == RTE_BEFORE )
> + {
> + after_index = index_rel;
> + }
> + if(strcmp(bef->eref->aliasname,"before") == 0 &&
> bef->rtekind == RTE_BEFORE )
> + {
> + before_index = index_rel;
> + }
> + index_rel++;
> + }
> /*
> * We can perform the desired Var fixup by abusing the
> fix_join_expr
> * machinery that formerly handled inner indexscan fixup. We
> search the
> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
> resultRelation,
> rtoffset);
>
> + bind_returning_variables(rlist, before_index, after_index);
> pfree(itlist);
>
> return rlist;
> ----------------------------------------
>
> Why is it enough to keep the last before_index and after_index values?
> What if there are more than one matching RangeTblEntry for "before"
> and/or for "after"? Is it an error condition or of them should be fixed?
I think it is safe, it is the first and the last index. On each level of
statement there can be (at most) the only one "before" and one "after"
alias.
Regards,
Karol Trzcionka

Hi,

2013-08-21 20:52 keltezéssel, Karol Trzcionka írta:
> W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
>> With this fixed, a more complete review:
> Thanks.
>> There is a new regression test (returning_before_after.sql) covering
>> this feature. However, I think it should be added to the group
>> where "returning.sql" resides currently. There is a value in running it
>> in parallel to other tests. Sometimes a subtle bug is uncovered
>> because of this and your v2 patch fixed such a bug already.
> Modified to work correct in parallel testing
>> doc/src/sgml/ref/update.sgml describes this feature.
>>
>> doc/src/sgml/dml.sgml should also be touched to cover this feature.
> I don't think so, there is not any info about returning feature, I think it shouldn't be
> part of my patch.
>> In the src/test/regress/parallel_schedule contains an extra
>> new line at the end, it shouldn't.
> Fixed
>>
>> In b/src/backend/optimizer/plan/setrefs.c:
>>
>> +static void bind_returning_variables(List *rlist, int bef, int aft);
>>
>> but later it becomes not public:
>>
>> + */
>> +void bind_returning_variables(List *rlist, int bef, int aft)
>> +{
> I've change to static in the definition.
>>
>> +extern void addAliases(ParseState *pstate);
>>
>> +void addAliases(ParseState *pstate)
>>
>> This external declaration is not needed since it is already
>> in src/include/parser/parse_clause.h
> Removed.
>>
>> In setrefs.c, bind_returning_variables() I would also rename
>> the function arguments, so "before" and "after" are spelled out.
>> These are not C keywords.
> Changed.
>> Some assignments, like:
>>
>> + var=(Var*)tle;
>> and
>> + int index_rel=1;
>>
>> in setrefs.c need spaces.
>>
>> "if()" statements need a space before the "(" and not after.
>>
>> Add spaces in the {} list in addAliases():
>> + char *aliases[] = {"before","after"};
>> like this: { "before", "after" }
>>
>> Spaces are needed here, too:
>> + for(i=0 ; i<noal; i++)
>>
>> This "noal" should be "naliases" or "n_aliases" if you really want
>> a variable. I would simply use the constant "2" for the two for()
>> loops in addAliases() instead, its purpose is obvious enough.
> Added some whitespaces.
>> In setrefs.c, bind_returning_variables():
>> + Var *var = NULL;
>> + foreach(temp, rlist){
>> Add an empty line after the declaration block.
> Added.
>> Other comments:
>>
>> This #define in pg_class:
>>
>> diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
>> index 49c4f6f..1b09994 100644
>> --- a/src/include/catalog/pg_class.h
>> +++ b/src/include/catalog/pg_class.h
>> @@ -154,6 +154,7 @@ DESCR("");
>> #define RELKIND_COMPOSITE_TYPE 'c' /* composite type */
>> #define RELKIND_FOREIGN_TABLE 'f' /* foreign table */
>> #define RELKIND_MATVIEW 'm' /* materialized view */
>> +#define RELKIND_BEFORE 'b' /* virtual table for
>> before/after statements */
>>
>> #define RELPERSISTENCE_PERMANENT 'p' /* regular table */
>> #define RELPERSISTENCE_UNLOGGED 'u' /* unlogged permanent
>> table */
>>
> RELKIND_BEFORE removed - it probably left over previous work and/or I needed it because
> RTE_BEFORE wasn't available (not included?).
>> Speaking of which, RTE_BEFORE is more properly named
>> RTE_RETURNING_ALIAS or something like that because it
>> covers both "before" and "after". Someone may have a better
>> idea for naming this symbol.
> Renamed to RTE_ALIAS - similar to addAliases (not addReturningAliases)

Others may have also a word in this topic, but consider that
this is *not* a regular alias and for those, RTE_ALIAS is not used,
like in

UPDATE table AS alias SET ...

Maybe RTE_RETURNING_ALIAS takes a little more typing, but
it becomes obvious when reading and new code won't confuse
it with regular aliases.

>> One question, though, about this part:
>>
>> ----------------------------------------
>> @@ -1900,7 +1954,27 @@ set_returning_clause_references(PlannerInfo *root,
>> int rtoffset)
>> {
>> indexed_tlist *itlist;
>> + int after_index=0, before_index=0;
>> + Query *parse = root->parse;
>>
>> + ListCell *rt;
>> + RangeTblEntry *bef;
>> +
>> + int index_rel=1;
>> +
>> + foreach(rt,parse->rtable)
>> + {
>> + bef = (RangeTblEntry *)lfirst(rt);
>> + if(strcmp(bef->eref->aliasname,"after") == 0 && bef->rtekind ==
>> RTE_BEFORE )
>> + {
>> + after_index = index_rel;
>> + }
>> + if(strcmp(bef->eref->aliasname,"before") == 0 && bef->rtekind ==
>> RTE_BEFORE )
>> + {
>> + before_index = index_rel;
>> + }
>> + index_rel++;
>> + }
>> /*
>> * We can perform the desired Var fixup by abusing the fix_join_expr
>> * machinery that formerly handled inner indexscan fixup. We search the
>> @@ -1924,6 +1998,7 @@ set_returning_clause_references(PlannerInfo *root,
>> resultRelation,
>> rtoffset);
>>
>> + bind_returning_variables(rlist, before_index, after_index);
>> pfree(itlist);
>>
>> return rlist;
>> ----------------------------------------
>>
>> Why is it enough to keep the last before_index and after_index values?
>> What if there are more than one matching RangeTblEntry for "before"
>> and/or for "after"? Is it an error condition or of them should be fixed?
> I think it is safe, it is the first and the last index. On each level of statement there
> can be (at most) the only one "before" and one "after" alias.

I deduced it in the meantime. I still think it worth a comment
in setrefs.c.

I think your v9 patch can be looked at by a more seasoned reviewer
or a committer.

Best regards,
Zoltán Böszörményi

> Regards,
> Karol Trzcionka
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

I took a look at this patch today, and I'm pretty skeptical about
whether it's on the right track. It adds a new kind of RTE called
RTE_ALIAS, which doesn't seem particularly descriptive and alias is
used elsewhere to mean something fairly different. More generally,
I'm not convinced that adding a new type of RTE is the right way to
handle this. The changes in pull_up_subqueries_recurse,
pullup_replace_vars_callback, preprocess_targetlist, and
build_joinrel_tlist seem like weird hacks. Those functions aren't
directly related to this feature; why do they need to know about it?

I wonder if we shouldn't be trying to handle resolution of these names
at an earlier processing stage, closer to the processor. I notice
that set_returning_clause_references() seems to have already solved
the hard part of this problem, which is frobbing target list entries
to return values from the new row rather than, as they naturally
would, the old row. In fact, we can already get approximately the
desired effect already:

rhaas=# update foo as after set a = before.a + 1 from foo as before
where before.a = after.a returning before.a, after.a;
a | a
---+---
1 | 2
(1 row)

Now this is a hack, because we don't really want to add an extra
scan/join just to get the behavior we want. But it seems to me
significant that this processing makes Vars that refer to the target
table refer to the new values, and if we got rid of it, they'd refer
to the old values. Can't we contrive to make AFTER.x parse into the
same Var node that x currently does? Then we don't need an RTE for
it. And maybe BEFORE.x ought to parse to the same node that just
plain x does but with some marking, or some other node wrapped around
it (like a TargetEntry with some flag set?) that suppresses this
processing. I'm just shooting from the hip here; that might be wrong
in detail, or even in broad strokes, but it just looks to me like the
additional RTE kind is going to bleed into a lot of places.

This patch also has significant style issues. Conforming to
PostgreSQL coding style is essential; if neither the author nor the
reviewer fixes problems in this area, then that is essentially making
it the committer's job, and the committer may not feel like taking
time to do that. Here's a selection of issues that I noticed while
reading this through: we use spaces around operators; the patch adds
two blank lines that shouldn't be there to the middle of the variable
declarations section; variables should be declared in the innermost
possible scope; single-statement blocks shouldn't have curly braces;
there shouldn't be whitespace before a closing parenthesis; there
should be a space after if and before the subsequent parenthesis;
braces should be uncuddled; code that does non-obvious things isn't
commented.

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

W dniu 04.10.2013 00:28, Robert Haas pisze:
> I wonder if we shouldn't be trying to handle resolution of these names
> at an earlier processing stage, closer to the processor.
Maybe it can be done in parser (in flex?) but at now it seems to be more
isolated feature.
> In fact, we can already get approximately the
> desired effect already:
>
> rhaas=# update foo as after set a = before.a + 1 from foo as before
> where before.a = after.a returning before.a, after.a;
> a | a
> ---+---
> 1 | 2
> (1 row)
Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched"
workflow. I can see significant difference. And your "after" returns the
value after whole the work (after trigger fired) as I know (I don't know
if it is needed or not, I only point at the difference).
> Now this is a hack, because we don't really want to add an extra
> scan/join just to get the behavior we want. But it seems to me
> significant that this processing makes Vars that refer to the target
> table refer to the new values, and if we got rid of it, they'd refer
> to the old values. Can't we contrive to make AFTER.x parse into the
> same Var node that x currently does? Then we don't need an RTE for
> it. And maybe BEFORE.x ought to parse to the same node that just
> plain x does but with some marking, or some other node wrapped around
> it (like a TargetEntry with some flag set?) that suppresses this
> processing. I'm just shooting from the hip here; that might be wrong
> in detail, or even in broad strokes, but it just looks to me like the
> additional RTE kind is going to bleed into a lot of places.
While planning/analyzing the problem there were many ideas about hot to
solve it. I was trying to avoid adding new RTE and referencing to "core"
table. However it makes more and more issues. You can see some PoC on
the
https://github.com/davidfetter/postgresql_projects/compare/returning_before_after
(other ideas I revert w/o commit because I couldn't get expected
result). The other major reason was that we can avoid touching executor
and/or parser's core (flex) this way. One observation: why shouldn't we
use the values computed at the moment (it would be computed again if we
want to do it later, in executor)?
I think we can do it by modify the Var structure (add some kind of flag
while generating the vars in parser?) but I'm not sure if it is good
idea. The major issue is to know if the Var/TargetEntry references to
the real alias "BEFORE" (named with "AS" syntax or even the real
table-name - I can see there is no difference in code) or the virtual
(from feature patch) "BEFORE". Doing it in parser (more "low-level")
would be very awful - we'd need to check in which part of statement
BEFORE/AFTER is placed (it is not allowed to use it in the other places
than in "RETURNING"). We don't want to make "BEFORE" and "AFTER"
restricted keywords.
Now most of the code means "don't touch these because they are not real" :)
If anyone has the fresh idea to it better, please write it by mail, I
don't have more ideas how to solve it.
> This patch also has significant style issues.
I'll try to fix it soon.
Regards,
Karol Trzcionka

On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka <karlikt(at)gmail(dot)com> wrote:
> Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched"
> workflow. I can see significant difference. And your "after" returns the
> value after whole the work (after trigger fired) as I know (I don't know
> if it is needed or not, I only point at the difference).

Sure, I'm not saying we should implement it that way. I'm just
pointing out that the ability already exists, at the executor level,
to return either tuple. So I think the executor itself shouldn't need
to be changed; it's just a matter of getting the correct plan tree to
pop out.

> While planning/analyzing the problem there were many ideas about hot to
> solve it.

Do you have a link to previous discussion on the mailing list?

> I think we can do it by modify the Var structure (add some kind of flag
> while generating the vars in parser?) but I'm not sure if it is good
> idea. The major issue is to know if the Var/TargetEntry references to
> the real alias "BEFORE" (named with "AS" syntax or even the real
> table-name - I can see there is no difference in code) or the virtual
> (from feature patch) "BEFORE". Doing it in parser (more "low-level")
> would be very awful - we'd need to check in which part of statement
> BEFORE/AFTER is placed (it is not allowed to use it in the other places
> than in "RETURNING"). We don't want to make "BEFORE" and "AFTER"
> restricted keywords.

You're right, it can't happen actually in the parser. But maybe it
can happen during parse analysis. I'd spend some time looking at
transformColumnRef(), because that's where we translate things x.y
into Var nodes. I'm not positive there's enough information available
at that stage, but if p_target_rangetblentry is populated at that
point, you should be able to make AFTER.x translate to a Var
referencing that range table entry. It's a bit less clear how we know
that we're inside the returning-list at that point; I'm not sure how
much work it would be to pass that information down. But I think it's
worth looking at.

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

W dniu 04.10.2013 02:51, Robert Haas pisze:
> Do you have a link to previous discussion on the mailing list?
Sorry, most of discussion was at IRC channel.
> I'm not positive there's enough information available
> at that stage, but if p_target_rangetblentry is populated at that
> point, you should be able to make AFTER.x translate to a Var
> referencing that range table entry.
It's not enough. Even if we know "where we are", there are more issues.
The main question is: how should we pass information about "hello, I'm
specific Var, don't evaluate me like others"? We can add two fields to
Var structure (flag - normal/before/after and no. column) - however it
needs to modify copyObject for Vars (at now it's done e.g. in
flatten_join_alias_vars_mutator for varoattno and varnoold). If
copyObject is modified, sure code in
flatten_join_alias_vars_mutator/pullup_replace_vars_callback will be
useless. I don't know if modifying pg at the low-level (changing
structure of Var and behaviour of copyObject) is good idea. Yes if the
community really want it but it needs more "votes". There is "medium"
solution: changing Var structure and do the "copy" like now (in mutator
and callback) but w/o the condition statement (for the new fields). I
think it might need to modify more places in code because of "comparing"
vars (maybe we'd need to include new fields while comparision).
Regards,
Karol Trzcionka

On 10/4/13 12:28 AM, Robert Haas wrote:
> In fact, we can already get approximately the
> desired effect already:
>
> rhaas=# update foo as after set a = before.a + 1 from foo as before
> where before.a = after.a returning before.a, after.a;
> a | a
> ---+---
> 1 | 2
> (1 row)
>
> Now this is a hack, because we don't really want to add an extra
> scan/join just to get the behavior we want. But it seems to me
> significant that this processing makes Vars that refer to the target
> table refer to the new values, and if we got rid of it, they'd refer
> to the old values. Can't we contrive to make AFTER.x parse into the
> same Var node that x currently does? Then we don't need an RTE for
> it.

This part sounds reasonable from here.

> And maybe BEFORE.x ought to parse to the same node that just
> plain x does but with some marking, or some other node wrapped around
> it (like a TargetEntry with some flag set?) that suppresses this
> processing. I'm just shooting from the hip here; that might be wrong
> in detail, or even in broad strokes, but it just looks to me like the
> additional RTE kind is going to bleed into a lot of places.

I might be completely in the woods here, but I believe something like
this was attempted by Karol earlier, and it failed if two concurrent
transactions did something similar to:

UPDATE foo SET a = a + 1 RETURNING BEFORE.a;

Both of them would see BEFORE.a = 0, because that's what the "a"
evaluated to from the tuple we got before EvalPlanQual.

But maybe what you're suggesting doesn't have this problem?

Regards,
Marko Tiikkaja

On 2013-10-03 20:51:08 -0400, Robert Haas wrote:
> On Thu, Oct 3, 2013 at 7:54 PM, Karol Trzcionka <karlikt(at)gmail(dot)com> wrote:
> > Compare EXPLAIN ANALYZE VERBOSE on your statement and on "patched"
> > workflow. I can see significant difference. And your "after" returns the
> > value after whole the work (after trigger fired) as I know (I don't know
> > if it is needed or not, I only point at the difference).
>
> Sure, I'm not saying we should implement it that way. I'm just
> pointing out that the ability already exists, at the executor level,
> to return either tuple. So I think the executor itself shouldn't need
> to be changed; it's just a matter of getting the correct plan tree to
> pop out.

Note what pullups ExecDelete is doing to return the old tuple
though... So, based on precedent special executor support is not an
unlikely thing to be required for a proper implemenation. As Marko
mentions, any trivial implementation not doing playing dirty like that
will refer to the wrong version of the tuple.

Greetings,

Andres Freund

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

On Fri, Oct 4, 2013 at 4:42 AM, Karol Trzcionka <karlikt(at)gmail(dot)com> wrote:
> W dniu 04.10.2013 02:51, Robert Haas pisze:
>> Do you have a link to previous discussion on the mailing list?
> Sorry, most of discussion was at IRC channel.
>> I'm not positive there's enough information available
>> at that stage, but if p_target_rangetblentry is populated at that
>> point, you should be able to make AFTER.x translate to a Var
>> referencing that range table entry.
> It's not enough. Even if we know "where we are", there are more issues.
> The main question is: how should we pass information about "hello, I'm
> specific Var, don't evaluate me like others"?

My point is that AFTER.x doesn't appear to need any special marking;
it means the same thing as target_table.x. BEFORE.x *does* need some
kind of special marking, and I admit I'm not sure what that should
look like. Maybe an RTE is OK, but letting that RTE get into the join
planning machinery does not seem good; that's going to result in funky
special cases all over the place.

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

On Fri, Oct 4, 2013 at 5:04 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> I might be completely in the woods here, but I believe something like this
> was attempted by Karol earlier, and it failed if two concurrent transactions
> did something similar to:
>
> UPDATE foo SET a = a + 1 RETURNING BEFORE.a;
>
> Both of them would see BEFORE.a = 0, because that's what the "a" evaluated
> to from the tuple we got before EvalPlanQual.
>
> But maybe what you're suggesting doesn't have this problem?

Hmm, it probably does. That explains why there are executor changes
here; I guess they need some comments to explain their purpose.

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

On Fri, Oct 04, 2013 at 10:42:32AM +0200, Karol Trzcionka wrote:
> W dniu 04.10.2013 02:51, Robert Haas pisze:
> > Do you have a link to previous discussion on the mailing list?
> Sorry, most of discussion was at IRC channel.
> > I'm not positive there's enough information available
> > at that stage, but if p_target_rangetblentry is populated at that
> > point, you should be able to make AFTER.x translate to a Var
> > referencing that range table entry.
> It's not enough. Even if we know "where we are", there are more issues.
> The main question is: how should we pass information about "hello, I'm
> specific Var, don't evaluate me like others"? We can add two fields to
> Var structure (flag - normal/before/after and no. column) - however it
> needs to modify copyObject for Vars (at now it's done e.g. in
> flatten_join_alias_vars_mutator for varoattno and varnoold). If
> copyObject is modified, sure code in
> flatten_join_alias_vars_mutator/pullup_replace_vars_callback will be
> useless. I don't know if modifying pg at the low-level (changing
> structure of Var and behaviour of copyObject) is good idea. Yes if the
> community really want it but it needs more "votes". There is "medium"
> solution: changing Var structure and do the "copy" like now (in mutator
> and callback) but w/o the condition statement (for the new fields). I
> think it might need to modify more places in code because of "comparing"
> vars (maybe we'd need to include new fields while comparision).
> Regards,
> Karol Trzcionka

Karol,

Do you plan to continue this work for the current commitfest? A lot
of people really want the feature :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Aug 21, 2013 at 08:52:25PM +0200, Karol Trzcionka wrote:
> W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> > With this fixed, a more complete review:
> Thanks.

I've done some syntactic and white space cleanup, here attached.

Karol, would you care to help with commenting the sections that need
same?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Feb 02, 2014 at 02:52:42PM -0800, David Fetter wrote:
> On Wed, Aug 21, 2013 at 08:52:25PM +0200, Karol Trzcionka wrote:
> > W dniu 21.08.2013 19:17, Boszormenyi Zoltan pisze:
> > > With this fixed, a more complete review:
> > Thanks.
>
> I've done some syntactic and white space cleanup, here attached.
>
> Karol, would you care to help with commenting the sections that need
> same?

Karol,

Thanks for the updates :)

Other folks,

Next version attached.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi,

Some comments about the patch:
* Coding Style:
* multiline comments have both /* and */ on their own lines.
* I think several places indent by two tabs.
* Spaces around operators
* ...
* Many of the new comments would enjoy a bit TLC by a native speaker.

* The way RTE_ALIAS creeps in many place where it doesn't seem to belong
seems to indicate that the design isn't yet ready. I share Robert's
suspicion that this would be better solved by referring to a special
range table entry.

Based on the lack of activity around this and the fact that this needs a
*significant* chunk of work before being committable, I am going to mark
this as returned with feedback.

Greetings,

Andres Freund

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

Andres Freund <andres <at> anarazel.de> writes:

>
> Hi,
>
> Some comments about the patch:
> * Coding Style:
> * multiline comments have both /* and */ on their own lines.
> * I think several places indent by two tabs.
> * Spaces around operators
> * ...
> * Many of the new comments would enjoy a bit TLC by a native speaker.
>
> * The way RTE_ALIAS creeps in many place where it doesn't seem to belong
> seems to indicate that the design isn't yet ready. I share Robert's
> suspicion that this would be better solved by referring to a special
> range table entry.
>
> Based on the lack of activity around this and the fact that this needs a
> *significant* chunk of work before being committable, I am going to mark
> this as returned with feedback.

I'm actively working towards converting our software at work to use Pg
instead of SQL Server and before we switch we'll need this feature to be
merged. I'll do what I can to get the verbage and style whipped into shape,
though I doubt I can do much with the actual code. Hopefully I can get
something in soon.