gSoC - ADD MERGE COMMAND - code patch submission

On Fri, Jul 9, 2010 at 10:25 PM, Boxuan Zhai <bxzhai2010@gmail.com> wrote:

Dear All,

This is ZHAI BOXUAN, a student of gSoC 2010. My project is to add merge
command in postgres.

This is the first submission of our codes, which has finished the parser,
analyzer and rewriter parts.

If you are interested in this project, please download the source code in
attachment and have test.

There are 3 files in the attachement. Two .patch file is created on the
/src/backend and /src/include folders (between orignial psql 8.4.3 and our
modified edition).

There is a more detailed instruction in readme.

Any comments will be highly appreciated.

Is there any chance you can submit this as a single patch file? Or if
not, can you at least use a zip or tar file instead of a RAR archive?

Ideally the patch would be against CVS HEAD, not 8.4.3.

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

On Fri, Jul 09, 2010 at 11:33:04PM -0400, Robert Haas wrote:

On Fri, Jul 9, 2010 at 10:25 PM, Boxuan Zhai <bxzhai2010@gmail.com> wrote:

Dear All,

This is ZHAI BOXUAN, a student of gSoC 2010. My project is to add merge
command in postgres.
There is a more detailed instruction in readme.

I would find it helpfull to find a short recap of how you want to
handle the various problems (mostly around locking) in the readme.

Any comments will be highly appreciated.

Is there any chance you can submit this as a single patch file? Or if
not, can you at least use a zip or tar file instead of a RAR archive?
Ideally the patch would be against CVS HEAD, not 8.4.3.

I would also suggest you base your patch either against the git tree
or CVS. Currently it does include patches agains generated files like
gram.y or kwlist.h which make it harder to see the real changes.

Thanks,
Andres

On Sat, Jul 10, 2010 at 11:52:31AM +0200, Andres Freund wrote:

On Fri, Jul 09, 2010 at 11:33:04PM -0400, Robert Haas wrote:

On Fri, Jul 9, 2010 at 10:25 PM, Boxuan Zhai <bxzhai2010@gmail.com> wrote:

Dear All,

This is ZHAI BOXUAN, a student of gSoC 2010. My project is to add merge
command in postgres.
There is a more detailed instruction in readme.

I would find it helpfull to find a short recap of how you want to
handle the various problems (mostly around locking) in the readme.

Any comments will be highly appreciated.

Is there any chance you can submit this as a single patch file? Or if
not, can you at least use a zip or tar file instead of a RAR archive?
Ideally the patch would be against CVS HEAD, not 8.4.3.

I would also suggest you base your patch either against the git tree
or CVS. Currently it does include patches agains generated files like
gram.y or kwlist.h which make it harder to see the real changes.

Thanks,
Andres

Please find enclosed a patch against git master as of
7b2668159bb4d0f5177a23d05bf7c2ab00bc0d75. It works up to make, but
fails on make check.

I'm thinking the docs for INSERT, UPDATE, and DELETE should link to
the docs for this, as they get written.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.org> writes:

Please find enclosed a patch against git master as of
7b2668159bb4d0f5177a23d05bf7c2ab00bc0d75. It works up to make, but
fails on make check.

There seem to be about four different comment styles used in this patch,
none of which match the project standard:
http://developer.postgresql.org/pgdocs/postgres/source-format.html

BTW, I notice that that page fails to mention anything about preferred
window width. I believe the project standard is to make things readable
in an 80-column window --- anyone have an objection to stating that
explicitly?

regards, tom lane

On Jul 10, 2010, at 11:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Fetter <david@fetter.org> writes:

Please find enclosed a patch against git master as of
7b2668159bb4d0f5177a23d05bf7c2ab00bc0d75. It works up to make, but
fails on make check.

There seem to be about four different comment styles used in this patch,
none of which match the project standard:
http://developer.postgresql.org/pgdocs/postgres/source-format.html

BTW, I notice that that page fails to mention anything about preferred
window width. I believe the project standard is to make things readable
in an 80-column window --- anyone have an objection to stating that
explicitly?

I certainly don't.

Though, if the worst problem with this patch is the formatting, we're doing *quite* well.

...Robert

On Sat, Jul 10, 2010 at 09:26:38AM -0700, David Fetter wrote:

On Sat, Jul 10, 2010 at 11:52:31AM +0200, Andres Freund wrote:

On Fri, Jul 09, 2010 at 11:33:04PM -0400, Robert Haas wrote:

On Fri, Jul 9, 2010 at 10:25 PM, Boxuan Zhai <bxzhai2010@gmail.com> wrote:

Dear All,

This is ZHAI BOXUAN, a student of gSoC 2010. My project is to add merge
command in postgres.
There is a more detailed instruction in readme.

I would find it helpfull to find a short recap of how you want to
handle the various problems (mostly around locking) in the readme.

Any comments will be highly appreciated.

Is there any chance you can submit this as a single patch file? Or if
not, can you at least use a zip or tar file instead of a RAR archive?
Ideally the patch would be against CVS HEAD, not 8.4.3.

I would also suggest you base your patch either against the git tree
or CVS. Currently it does include patches agains generated files like
gram.y or kwlist.h which make it harder to see the real changes.

Please find enclosed a patch against git master as of
7b2668159bb4d0f5177a23d05bf7c2ab00bc0d75. It works up to make, but
fails on make check.

I'm thinking the docs for INSERT, UPDATE, and DELETE should link to
the docs for this, as they get written.

Cheers,
David.

Oops. Just noticed that there were 56 lines' worth of C++ style
comments, which I've corrected in the enclosed patch, along with some
spelling mistakes, grammar, and gratuitous white space.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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

Robert Haas <robertmhaas@gmail.com> writes:

Though, if the worst problem with this patch is the formatting, we're doing *quite* well.

Well, the worst problem with it is that it hasn't touched the
interesting part, ie, what happens at execution time. I haven't
seen a design for that, which means it's impossible to evaluate
whether the code that is here is of any use. We might need some
other representation entirely.

BTW, Fetter's version of the patch seems to be lacking any gram.y
changes, but surely those exist already?

regards, tom lane

On Sat, Jul 10, 2010 at 01:18:49PM -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Though, if the worst problem with this patch is the formatting, we're doing *quite* well.

Well, the worst problem with it is that it hasn't touched the
interesting part, ie, what happens at execution time. I haven't
seen a design for that, which means it's impossible to evaluate
whether the code that is here is of any use. We might need some
other representation entirely.

BTW, Fetter's version of the patch seems to be lacking any gram.y
changes, but surely those exist already?

Oops.

Fixed that now in attached patch.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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 Sat, Jul 10, 2010 at 10:39:02AM -0700, David Fetter wrote:

On Sat, Jul 10, 2010 at 01:18:49PM -0400, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Though, if the worst problem with this patch is the formatting, we're doing *quite* well.

Well, the worst problem with it is that it hasn't touched the
interesting part, ie, what happens at execution time. I haven't
seen a design for that, which means it's impossible to evaluate
whether the code that is here is of any use. We might need some
other representation entirely.

BTW, Fetter's version of the patch seems to be lacking any gram.y
changes, but surely those exist already?

Oops.

Fixed that now in attached patch.

By the way, "make check" fails here with attached initdb.log:

./pg_regress --inputdir=. --dlpath=. --multibyte=SQL_ASCII --temp-install=./tmp_check --top-builddir=../../.. --schedule=./parallel_schedule
============== creating temporary installation ==============
============== initializing database system ==============

pg_regress: initdb failed
Examine /home/shackle/pggit/postgresql/src/test/regress/log/initdb.log for the reason.
Command was: "/home/shackle/pggit/postgresql/src/test/regress/./tmp_check/install//home/shackle/tip/bin/initdb" -D "/home/shackle/pggit/postgresql/src/test/regress/./tmp_check/data" -L "/home/shackle/pggit/postgresql/src/test/regress/./tmp_check/install//home/shackle/tip/share/postgresql" --noclean > "/home/shackle/pggit/postgresql/src/test/regress/log/initdb.log" 2>&1
make[2]: *** [check] Error 2
make[2]: Leaving directory `/home/shackle/pggit/postgresql/src/test/regress'
make[1]: *** [check] Error 2
make[1]: Leaving directory `/home/shackle/pggit/postgresql/src/test'
make: *** [check] Error 2

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.org> writes:

By the way, "make check" fails here with attached initdb.log:

creating system views ... FATAL: unrecognized token: "false"

Hm, I'd suspect something fouled up in keyword recognition.
Did you do a "make clean" and rebuild?

BTW, this patch is still a few bricks shy of a load, since there's no
kwlist.h change and so the new MERGE keyword couldn't possibly be
recognized. More generally, I'm wondering why the original .rar
submission was 300k (presumably compressed) and your diff is only
about 35k ...

regards, tom lane

On lör, 2010-07-10 at 09:26 -0700, David Fetter wrote:

Please find enclosed a patch against git master as of
7b2668159bb4d0f5177a23d05bf7c2ab00bc0d75. It works up to make, but
fails on make check.

It looks like this implementation reaches about the same level of parser
support as the stuff that I had coded up a few months ago at the airport
within a couple of hours [0]http://git.postgresql.org/gitweb?p=users/petere/postgresql.git;a=shortlog;h=refs/heads/merge-statement[1]http://petereisentraut.blogspot.com/2010/05/merge-syntax.html, and I had sent the student the code, so
he could have had that for free.

But as others had commented already, the meat of the problem is how
MERGE statement *execution* is supposed to work.

[0]: http://git.postgresql.org/gitweb?p=users/petere/postgresql.git;a=shortlog;h=refs/heads/merge-statement
http://git.postgresql.org/gitweb?p=users/petere/postgresql.git;a=shortlog;h=refs/heads/merge-statement
[1]: http://petereisentraut.blogspot.com/2010/05/merge-syntax.html

On Sat, Jul 10, 2010 at 01:53:53PM -0400, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

By the way, "make check" fails here with attached initdb.log:

creating system views ... FATAL: unrecognized token: "false"

Hm, I'd suspect something fouled up in keyword recognition. Did you
do a "make clean" and rebuild?

I did make maintainer-clean.

BTW, this patch is still a few bricks shy of a load, since there's
no kwlist.h change and so the new MERGE keyword couldn't possibly be
recognized. More generally, I'm wondering why the original .rar
submission was 300k (presumably compressed) and your diff is only
about 35k ...

I'll look into that. From what you can see, is it worth trying to
clean up, starting from base, or should we just wait for the next
revision of the patch?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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@fetter.org> writes:

On Sat, Jul 10, 2010 at 01:53:53PM -0400, Tom Lane wrote:

BTW, this patch is still a few bricks shy of a load, since there's
no kwlist.h change and so the new MERGE keyword couldn't possibly be
recognized. More generally, I'm wondering why the original .rar
submission was 300k (presumably compressed) and your diff is only
about 35k ...

I'll look into that. From what you can see, is it worth trying to
clean up, starting from base, or should we just wait for the next
revision of the patch?

Well, rebasing against HEAD will presumably help the submitter
(assuming that he takes the advice to work against HEAD not 8.4.x).
But really what we need to see is design documentation, not code.

regards, tom lane

Tom Lane wrote:

BTW, I notice that that page fails to mention anything about preferred
window width. I believe the project standard is to make things readable
in an 80-column window --- anyone have an objection to stating that
explicitly?

No, on the contrary, I'm in favor of stating it.

cheers

andrew

On lör, 2010-07-10 at 12:45 -0400, Tom Lane wrote:

I believe the project standard is to make things readable
in an 80-column window --- anyone have an objection to stating that
explicitly?

Is that what pgindent reformats it to?

Peter Eisentraut <peter_e@gmx.net> writes:

On lör, 2010-07-10 at 12:45 -0400, Tom Lane wrote:

I believe the project standard is to make things readable
in an 80-column window --- anyone have an objection to stating that
explicitly?

Is that what pgindent reformats it to?

pgindent tries to leave a character or two to spare, IIRC, so its target
is probably 78 or thereabouts.

regards, tom lane

Hi,

Thanks for all these feedback.

I found that people have problems on running my codes, which probably comes
from my nonstandard submission format. I can compile, install and initialize
postgres in my own machine. The system accepts MERGE command and will throw
an error when it runs into the executor, which cannot recognize the MERGE
command type so far.

I will make a standard patch as soon as possible. Sorry for the troubles.

Yours Boxuan

2010/7/11 Tom Lane <tgl@sss.pgh.pa.us>

Boxuan Zhai wrote:

I found that people have problems on running my codes, which probably
comes from my nonstandard submission format. I can compile, install
and initialize postgres in my own machine. The system accepts MERGE
command and will throw an error when it runs into the executor, which
cannot recognize the MERGE command type so far.

Your job as a potential contributor to PostgreSQL is to make it as easy
as possible for others to test your code out and get good results. I
sent you some more detailed guidelines over the weekend as to what I
think you should do here to achieve that. You should wait until you've
gotten a private review from one of the two people who have volunteered
to help you out here before you submit anything else to the list.
Wasting the time of everyone in the community by sharing code that
doesn't mean any of the project guidelines is a very bad idea; please
don't do that again.

--
Greg Smith 2ndQuadrant US Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.us

On mån, 2010-07-12 at 10:04 -0400, Greg Smith wrote:

Wasting the time of everyone in the community by sharing code that
doesn't mean any of the project guidelines is a very bad idea; please
don't do that again.

I think it's better to share code that doesn't mean project guidelines
and solicit advice rather than not to share anything.