SEO

Brendan Jurd escribió:

> I'd like to submit my first version of this patch for review. I have
> introduced a new struct in print.h called printTableContent, which is
> used to compose the contents of a psql table. The methods exposed for
> this struct are as follows:

Looks cool -- on a first read, I think you should add some more code
comments at the top of each function specifying whether the texts need
to be translated by the caller or done by the function itself. Also it
would be good if it is consistent, too :-)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

On Tue, Apr 15, 2008 at 06:11:32PM +1000, Brendan Jurd wrote:
> Now my question is, how do I indicate that a second version of the
> patch has been submitted on the wiki? Should I leave the primary link
> pointing at the original submission, or update it to point at this
> message?

I'd say add a row: New version submitted (link) (brief summary of
changes)

If there are many more versions you can consider cutting, but you're
not there yet.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIBGaAIB7bNG8LQkwRAsfqAJ95yX6ICNzbokH3w6q7wa nK+YUe/QCfVQfS
NAMAID3NFW+wKrpH0Cw/aAY=
=ttk2
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera wrote:
> Brendan Jurd escribió:
>
> > The second version of the patch is attached.
>
> Thanks. I looked the patch over and did some minor changes. Modified
> version attached.
>

Cool, I had a look through your changes and they all seemed fine to
me. In particular, moving the header comments to the ... header ...
file seemed like a smart move. =)

> One thing I'm not entirely happy about is the fact that we need a
> pointer to the last footer/cell/header, so two pointers for each element
> kind.
>

Well, the alternative is iterating through the array each time you
want to add something until you hit a NULL pointer, and adding the new
item at that point. Considering we're only chewing up an extra 4 *
sizeof(pointer) = 16 bytes in the struct, it seems like a reasonable
price to pay for the convenience.

What is it about the extra fields that makes you unhappy?

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBo2y5YBsbHkuyV0RAmLhAJ9CP/9L1Nv7WbCtrCYu6tyoGhQItQCeMRm0
HegSSBq8tXw43Kj2xeQ2RCs=
=RvzP
-----END PGP SIGNATURE-----

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

Brendan Jurd escribió:

> On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera wrote:

> > One thing I'm not entirely happy about is the fact that we need a
> > pointer to the last footer/cell/header, so two pointers for each
> > element kind.
>
> Well, the alternative is iterating through the array each time you
> want to add something until you hit a NULL pointer, and adding the new
> item at that point. Considering we're only chewing up an extra 4 *
> sizeof(pointer) = 16 bytes in the struct, it seems like a reasonable
> price to pay for the convenience.

Well, consider that there are going to be 1 or 2 entries in the arrays
in most cases anyway :-) Well, as far as footers go anyway ... I just
realized that in all other cases it will certainly be the wrong thing to
do :-) Still, perhaps a integer count is better?

> What is it about the extra fields that makes you unhappy?

I don't know if "unnecessarity" is a word, but I hope you get what I
mean :-)

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, Apr 17, 2008 at 9:53 AM, Alvaro Herrera wrote:
>
> Well, consider that there are going to be 1 or 2 entries in the arrays
> in most cases anyway :-) Well, as far as footers go anyway ... I just
> realized that in all other cases it will certainly be the wrong thing to
> do :-) Still, perhaps a integer count is better?
>

I oscillated between using an integer and a pointer for ->header and
- ->cell while I was writing the code. In the end I went with pointer
simply because it makes the iterations nicer looking, and there's a
symmetry in having the "first item" and "last item" pointers be the
same type.

If there's some technical reason that integers would be better, I'll
all ears, but from an aesthetic/code readability perspective I like
the pointer.

> > Brendan Jurd escribió:
> > What is it about the extra fields that makes you unhappy?
>
> I don't know if "unnecessarity" is a word, but I hope you get what I
> mean :-)

Since the footers list is usually pretty short, you could make an
argument for dropping the "last footer" pointer and just iterating
through the list to find the last element every time you want to do
something with footers. And to do that, you'd have to declare a
temporary pointer inside the AddFooter function anyway, so you're not
getting rid of the pointer so much as making it slightly more
transient.

All that having been said, C isn't my native language. So if the
pointer is indeed wasteful I'm happy to cede to the wisdom of more
experienced C coders.

Cheers,
BJ
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIBw7m5YBsbHkuyV0RAv4IAJ0cKrziZpNWkVV7LxFhlV/V5L0pJACfUtZ+
cVbcjL1e89j21JDZJBVdBqw=
=Nzr+
-----END PGP SIGNATURE-----

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

Brendan Jurd escribió:

> On Thu, Apr 17, 2008 at 7:27 AM, Alvaro Herrera wrote:

> > Thanks. I looked the patch over and did some minor changes. Modified
> > version attached.
>
> Cool, I had a look through your changes and they all seemed fine to
> me. In particular, moving the header comments to the ... header ...
> file seemed like a smart move. =)

FWIW I just noticed something else. With this patch we add pg_strdup
calls into print.c. pg_strdup lives in common.c.

This is fine as psql is concerned, but we have another problem which is
that in bin/scripts there are two programs that want to use
printQuery(). The problem is that there's no pg_strdup there :-(

The easy solution is to add pg_strdup to bin/scripts/common.c, but there
we don't have a global progname, so the error report in the out of
memory case cannot carry the name of the program crashing.

I don't like that, but I don't see any other solution. Ideas welcome.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

Brendan Jurd escribió:

> Here's the latest version of the printTable API. This version is
> against the current HEAD and merges in the changes made by the
> recently committed psql wrap patch.
>
> This version also includes Alvaro's fix for the issue of pg_strdup not
> being available to programs in scripts/ (as quoted below).

Thanks. I had to merge Tom's fixes to the wrap code too.

Another thing that I changed is that now "not null" and "default %s" is
translatable in a table description. Also, I removed the _() call
around things like "?%c? \"%s.%s\"", which is what we use for tables of
which we don't recognize the relkind.

Thanks for the patch.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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