Prev: Need a Promotion for a good life. alamode adenia absonant
Next: [HACKERS] PostGIS vs. PGXS in 9.0beta3
From: Robert Haas on 27 Jul 2010 13:06
On Tue, Jul 27, 2010 at 1:04 AM, Boxuan Zhai <bxzhai2010(a)gmail.com> wrote:
> I have get a edition that the merge command can run. It accept the standard
> merge command and can do UPDATE, INSERT and DELETE actions now. But we
> cannot put additional qualification for actions. There are some bugs when we
> try to evaluate the quals which make the system quit. I will fix it soon.

This patch doesn't compile. You're using zbxprint() from a bunch of
places where it's not defined. I get compile warnings for all of
those files and then a link failure at the end. You might find it
useful to create src/Makefile.custom in your local tree and put
COPT=-Werror in there; it tends to prevent problems of this kind.

Undefined symbols:
"_zbxprint", referenced from:
_transformStmt in analyze.o
_ExecInitMergeAction in nodeModifyTable.o
_ExecModifyTable in nodeModifyTable.o
_ExecInitModifyTable in nodeModifyTable.o
_merge_action_planner in planner.o

Not that it's as high-priority as getting this fully working, but you
should revert the useless changes in this patch - e.g. the one-line
change to heaptuple.c is obvious debugging leftovers, and all of the
changes to execQual.c and execUtil.c are whitespace-only. You should
also try to make your code and comments conform to project style
guidelines. In general, you'll find it easier to keep track of your
changes (and you'll have fewer spurious changes) if you use git diff
master...yourbranch instead of marking comments, etc. with ZBX or
similar.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Boxuan Zhai on 28 Jul 2010 06:08
2010/7/28 Robert Haas <robertmhaas(a)gmail.com>

> On Tue, Jul 27, 2010 at 1:04 AM, Boxuan Zhai <bxzhai2010(a)gmail.com> wrote:
> > I have get a edition that the merge command can run. It accept the
> standard
> > merge command and can do UPDATE, INSERT and DELETE actions now. But we
> > cannot put additional qualification for actions. There are some bugs when
> we
> > try to evaluate the quals which make the system quit. I will fix it soon.
>
> This patch doesn't compile. You're using zbxprint() from a bunch of
> places where it's not defined. I get compile warnings for all of
> those files and then a link failure at the end. You might find it
> useful to create src/Makefile.custom in your local tree and put
> COPT=-Werror in there; it tends to prevent problems of this kind.
>
> Undefined symbols:
> "_zbxprint", referenced from:
> _transformStmt in analyze.o
> _ExecInitMergeAction in nodeModifyTable.o
> _ExecModifyTable in nodeModifyTable.o
> _ExecInitModifyTable in nodeModifyTable.o
> _merge_action_planner in planner.o
>
> Sorry, this is a debug function defined by me. It may not be included in
the patch. I add a line of "#define zbxprint printf" somewhere in the
system.


> Not that it's as high-priority as getting this fully working, but you
> should revert the useless changes in this patch - e.g. the one-line
> change to heaptuple.c is obvious debugging leftovers, and all of the
> changes to execQual.c and execUtil.c are whitespace-only. You should
> also try to make your code and comments conform to project style
> guidelines. In general, you'll find it easier to keep track of your
> changes (and you'll have fewer spurious changes) if you use git diff
> master...yourbranch instead of marking comments, etc. with ZBX or
> similar.
>
>

I will clean all these in my next patch.

I am now very confused with the failure of action qualification. I look
through the whole process of a query, from parser to executor. In my
opinion, the qualification transformed in analyzer, will be processed by
prepsocess_qual_condition() in planner, and then by the ExecInitExpr()
function in excutor start phase (in InitPlan() function). Then the qual is
ready to be used in ExecQual(). Am I correct?

I have done these on the merge action qual, but when I pass them into
ExecQual(), the server just closed abnormally. I don't know if I missed any
steps on preparing the qual expressions.

> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>
From: Robert Haas on 28 Jul 2010 06:28
On Wed, Jul 28, 2010 at 6:08 AM, Boxuan Zhai <bxzhai2010(a)gmail.com> wrote:
>> On Tue, Jul 27, 2010 at 1:04 AM, Boxuan Zhai <bxzhai2010(a)gmail.com> wrote:
>> > I have get a edition that the merge command can run. It accept the
>> > standard
>> > merge command and can do UPDATE, INSERT and DELETE actions now. But we
>> > cannot put additional qualification for actions. There are some bugs
>> > when we
>> > try to evaluate the quals which make the system quit. I will fix it
>> > soon.
>>
>> This patch doesn't compile. �You're using zbxprint() from a bunch of
>> places where it's not defined. �I get compile warnings for all of
>> those files and then a link failure at the end. �You might find it
>> useful to create src/Makefile.custom in your local tree and put
>> COPT=-Werror in there; it tends to prevent problems of this kind.
>>
>> Undefined symbols:
>> �"_zbxprint", referenced from:
>> � � �_transformStmt in analyze.o
>> � � �_ExecInitMergeAction in nodeModifyTable.o
>> � � �_ExecModifyTable in nodeModifyTable.o
>> � � �_ExecInitModifyTable in nodeModifyTable.o
>> � � �_merge_action_planner in planner.o
>>
> Sorry, this is a debug function defined by me. It may not be included in the
> patch. I add a line of "#define zbxprint printf" somewhere in the system.

Yeah, but it's not included in all the places that are needed to make
everything compile. You might move this to postgres.h or something.

>> Not that it's as high-priority as getting this fully working, but you
>> should revert the useless changes in this patch - e.g. the one-line
>> change to heaptuple.c is obvious debugging leftovers, and all of the
>> changes to execQual.c and execUtil.c are whitespace-only. �You should
>> also try to make your code and comments conform to project style
>> guidelines. �In general, you'll find it easier to keep track of your
>> changes (and you'll have fewer spurious changes) if you use git diff
>> master...yourbranch instead of marking comments, etc. with ZBX or
>> similar.
>
> I will clean all these in my next patch.
>
> I am now very confused with the failure of action qualification. I look
> through the whole process of a query, from parser to executor. In my
> opinion, the qualification transformed in analyzer, will be processed by
> prepsocess_qual_condition() in planner, and then by the ExecInitExpr()
> function in excutor start phase (in InitPlan() function). Then the qual is
> ready to be used in ExecQual(). Am I correct?

I'm not sure, sorry.

> I have done these on the merge action qual, but when I pass them into
> ExecQual(), the server just closed abnormally. I don't know if I missed any
> steps on preparing��the qual expressions.

Have you tried attaching a debugger? Try "SELECT pg_backend_pid()"
and then use "gdb -p the_pid" from another window. Hit "continue".
Then do whatever it is that's crashing. That way you can get a stack
backtrace, and poke around at the data structures.

Using pprint() on node-type data structures, either in debugging code
or actually straight from the debugger via "call", is also very
helpful, often-times.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 29 Jul 2010 16:51
On Wed, Jul 28, 2010 at 11:51 AM, Boxuan Zhai <bxzhai2010(a)gmail.com> wrote:
> I have fixed the action qual problem. Now the system can run merge command,
> with quals.
>
> I create a clean patch file (no debug clauses). See the attachment.
>
> Please try this new command if you have interest.

So, I tried this today, but:

- I got some compiler warnings in analyze.c, and
- when tried to run 'make check' with the patch applied, initdb failed.

So you still need to do some more bug-squashing on this...

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Greg Smith on 30 Jul 2010 12:21
Boxuan Zhai wrote:
> I create a clean patch file (no debug clauses). See the attachment.

Some general coding feedback for you on this.

It's not obvious to people who might want to try this out what exactly
they are supposed to do. Particularly for complicated patches like
this, where only a subset of the final feature might actually be
implemented, some sort of reviewer guide suggesting what should and
shouldn't work would be extremely helpful. I recall there was some sort
of patch design guide in an earlier version of this patch; it doesn't
seem to be there anymore. Don't remember if that had any examples in it.

Ultimately, the examples of working behavior for this patch will need to
be put into code. The way that happens is by adding working examples
into the regression tests for the database. If you had those done for
this patch, I wouldn't have to ask for code examples; I could just look
at the source to the regression output and see how to use it against the
standard database the regression samples create, and then execute
against. This at least lets you avoid having to generate some test
data, because there will already be some in the regression database for
you to use. There is an intro this topic at
http://wiki.postgresql.org/wiki/Regression_test_authoring Another
common way to generate test data is to run pgbench which creates 4
tables and populates them with data.

As far as the patch itself goes, you have some work left on cleaning
that up still you'll need to do eventually. What I would suggest is
actually reading the patch itself; don't just generate it and send it,
read through the whole thing like someone new to it would do. One way
you can improve what you've done already is to find places where you
have introduced changes to the code structure just through editing.
Here's an example of what I'm talking about, from line 499 of your patch:

- break;
+ break;

This happened because you added two invisible tabs to the end of this
line. This makes the patch larger for no good reason and tends to
infuriate people who work on the code. There's quite a bit of extra
lines added in here too that should go. You should consider reducing
the ultimate size of the patch in terms of lines a worthwhile use of
your time, even if it doesn't change how things work. There's lots of
examples in this one where you put two or three lines between two
statements when a single one would match the look of the code in that
file. A round of reading the diff looking for that sort of problem
would be useful.

Another thing you should do is limit how long each line is when
practical. You have lots of seriously wide comment lines here right now
in particular. While there are some longer lines in the PostgreSQL code
right now, that's code, not comments. And when you have a long line and
a long comment, don't tack the comment onto the end. Put it on the line
above instead. Also, don't use "//" in the middle of comments the way
you've done in a few places here.

Getting some examples sorted out and starting on regression tests is
more important than the coding style parts I think, just wanted to pass
those along while I noticed them reading the patch, so you could start
looking out for them more as you continue to work on it.

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3 4
Prev: Need a Promotion for a good life. alamode adenia absonant
Next: [HACKERS] PostGIS vs. PGXS in 9.0beta3