Syntax for partitioning
in [PgSql]
|
Prev: [HACKERS] pg_read_file() and non-ascii input file
Next: [HACKERS] Proposal - temporal contrib module
From: Peter Eisentraut on 20 Nov 2009 00:56 On fre, 2009-11-20 at 11:14 +0530, Nikhil Sontakke wrote: > Hi, > > >> > partinfo = (PartitionInfo *) malloc(ntups * sizeof(PartitionInfo)); > > > > 1) Please stop casting the results of palloc and malloc. We are not > > writing C++ here. > > > > I thought it was/is a good C programming practice to typecast (void *) > always to the returning structure type!! This could be preferable if you use sizeof on the type, so that you have an additional check that the receiving variable actually has that type. But if you use sizeof on the variable itself, it's unnecessary: You just declare the variable to be of some type earlier, and then the expression allocates ntups of it, without having to repeat the type information. > > Regards, > Nikhils > > > 2) I would prefer that you apply sizeof on the variable, not on the > > type. That way, the expression is independent of any type changes of > > the variable, and can be reviewed without having to scroll around for > > the variable definition. > > > > So how about, > > > > partinfo = palloc(ntups * sizeof(*partinfo)); > > > > > > -- > > Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) > > To make changes to your subscription: > > http://www.postgresql.org/mailpref/pgsql-hackers > > > > > > -- > http://www.enterprisedb.com > -- 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: Tom Lane on 20 Nov 2009 01:19 Nikhil Sontakke <nikhil.sontakke(a)enterprisedb.com> writes: >>> partinfo = (PartitionInfo *) malloc(ntups * sizeof(PartitionInfo)); >> >> 1) Please stop casting the results of palloc and malloc. �We are not >> writing C++ here. > I thought it was/is a good C programming practice to typecast (void *) > always to the returning structure type!! Yes. The above is good style because it ensures that the variable you're assigning the pointer to is the right type to match the sizeof computation. In C++ you'd use operator new instead and still have that type-check without the cast, but indeed we are not writing C++ here. The *real* bug in the quoted code is that it's using malloc. There are a few places in PG where it's appropriate to use malloc not palloc, but pretty darn few. regards, tom lane -- 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: Tom Lane on 20 Nov 2009 01:51 Peter Eisentraut <peter_e(a)gmx.net> writes: > 2) I would prefer that you apply sizeof on the variable, not on the > type. That way, the expression is independent of any type changes of > the variable, and can be reviewed without having to scroll around for > the variable definition. FWIW, I think the general project style has been the other way. Yes, it means you write the type name three times not once, but the other side of that coin is that it makes it more obvious what is happening (and gives you an extra chance to realize that the type you wrote is wrong ...) regards, tom lane -- 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: Simon Riggs on 20 Nov 2009 02:08 On Thu, 2009-11-19 at 10:53 -0500, Robert Haas wrote: > On Thu, Nov 19, 2009 at 9:58 AM, Markus Wanner <markus(a)bluegap.ch> wrote: > > Hi, > > > > Robert Haas wrote: > >> > >> Settling on a syntax, and an internal representation for that syntax, > > > > I've been under the impression that this was only about syntax. What are the > > internal additions? > > I haven't looked at it in detail, but it adds a new pg_partition > table. Whether that table is suitably structured for use by the > optimizer is not clear to me. If it does, then my review comments to Kedar still apply: * why do we want another catalog table? what's wrong with pg_inherits? It might need additional columns, and it certainly needs another index. * We need an internal data structure (discussed on this thread also). Leaving stuff in various catalog tables would not be the same thing at all. -- Simon Riggs www.2ndQuadrant.com -- 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 20 Nov 2009 10:59
On Fri, Nov 20, 2009 at 2:08 AM, Simon Riggs <simon(a)2ndquadrant.com> wrote: > On Thu, 2009-11-19 at 10:53 -0500, Robert Haas wrote: >> On Thu, Nov 19, 2009 at 9:58 AM, Markus Wanner <markus(a)bluegap.ch> wrote: >> > Hi, >> > >> > Robert Haas wrote: >> >> >> >> Settling on a syntax, and an internal representation for that syntax, >> > >> > I've been under the impression that this was only about syntax. What are the >> > internal additions? >> >> I haven't looked at it in detail, but it adds a new pg_partition >> table. Whether that table is suitably structured for use by the >> optimizer is not clear to me. > > If it does, then my review comments to Kedar still apply: > > * why do we want another catalog table? what's wrong with pg_inherits? > It might need additional columns, and it certainly needs another index. That might work, I haven't looked at it enough to be sure one way or the other. > * We need an internal data structure (discussed on this thread also). > Leaving stuff in various catalog tables would not be the same thing at > all. Ultimately I'm guessing that for query optimization we'll need to include the relevant info in the relcache. But I think that can wait until we're ready to actually make the optimizer changes - not much point in caching data that is never used. Right now I think it's enough to verify (which I haven't) that the schema of the catalog table is suitable for straightforward construction of the data that will eventually need to be cached. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |