SEO

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------


NikhilS wrote:
> Hi Neil,
>
> On 5/18/07, Neil Conway <neilc@samurai.com> wrote:
> >
> > On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote:
> > > Has a revised version of this patch been submitted?
> >
> > In the absence of a revised patch, I can finish the feature myself, but
> > I won't get the free cycles until after PGCon. I can commit to getting
> > it done before the end of May, or else we can just push this to 8.4.
>
>
> I had spent some time on this earlier so decided to complete and send the
> patch to you for review. This patch supports copying of expressions,
> predicates, opclass, amorder, reloptions etc. The test case also contains
> some more additions with this patch. Please let me know if there are any
> issues.
>
> Also, if this patch is acceptable, I think the mechanism provided here can
> be used to support "INCLUDING INDEXES" case easily too.
>
> Regards,
> Nikhils
> --
> EnterpriseDB http://www.enterprisedb.com

[ Attachment, skipping... ]

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

Bruce Momjian escribió:
>
> Your patch has been added to the PostgreSQL unapplied patches list at:
>
> http://momjian.postgresql.org/cgi-bin/pgpatches
>
> It will be applied as soon as one of the PostgreSQL committers reviews
> and approves it.

I noticed that this patch uses names for some things (for example it
gets the name of the access method), and then builds a IndexStmt which
contains the name. I don't think this is a good idea. I think what
should happen here is that the function to build indexes should be split
in two: one to resolve the names and fill a structure with Oids of
things, and another to get that structure and actually build the index
or constraint. For example look into how GrantStmt is turned into
InternalGrant, and the stuff in aclchk.c to work with that.

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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Hi,

>
> I noticed that this patch uses names for some things (for example it
> gets the name of the access method), and then builds a IndexStmt which
> contains the name. I don't think this is a good idea. I think what
> should happen here is that the function to build indexes should be split
> in two: one to resolve the names and fill a structure with Oids of
> things, and another to get that structure and actually build the index
> or constraint. For example look into how GrantStmt is turned into
> InternalGrant, and the stuff in aclchk.c to work with that.


The index creation happens after the new table (which is LIKE the parent)
has been created, by appending the cxt.alist information to "extras_after".
The entry point for the index creation is thus via ProcessUtility which
expects an IndexStmt structure. That is why the current patch does all the
Oid to name mapping exercise to populate the relevant fields in IndexStmt
some of which are char pointers. The internal DefineIndex function also
expects most of the fields to be in IndexStmt like format.

If we want to follow the above suggestion, as I understand it, we might have
to devise a new structure to contain Oids and make ProcessUtility accept a
new nodeTag. We will also not be able to use existing Index definition
functions and this will lead to more coding IMHO. Do we want to go down this
path? Or is there something else that has been suggested above and that I am
missing completely?

Please let me know.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

Hi,

> The index creation happens after the new table (which is LIKE the parent)
> has been created, by appending the cxt.alist information to
> "extras_after". The entry point for the index creation is thus via
> ProcessUtility which expects an IndexStmt structure. That is why the current
> patch does all the Oid to name mapping exercise to populate the relevant
> fields in IndexStmt some of which are char pointers. The internal
> DefineIndex function also expects most of the fields to be in IndexStmt like
> format.
>
> If we want to follow the above suggestion, as I understand it, we might
> have to devise a new structure to contain Oids and make ProcessUtility
> accept a new nodeTag. We will also not be able to use existing Index
> definition functions and this will lead to more coding IMHO. Do we want to
> go down this path? Or is there something else that has been suggested above
> and that I am missing completely?


OTOH, we can populate a new structure with the relevant Oids, IndexInfo
information from parent relation indexes and call index_create directly
from within ProcessUtility. Guess, it should be cleaner than the current
approach.

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

Hi,

On 5/23/07, NikhilS <nikkhils@gmail.com> wrote:
>
> Hi,
>
> > The index creation happens after the new table (which is LIKE the
> > parent) has been created, by appending the cxt.alist information to
> > "extras_after". The entry point for the index creation is thus via
> > ProcessUtility which expects an IndexStmt structure. That is why the current
> > patch does all the Oid to name mapping exercise to populate the relevant
> > fields in IndexStmt some of which are char pointers. The internal
> > DefineIndex function also expects most of the fields to be in IndexStmt like
> > format.
> >
> > If we want to follow the above suggestion, as I understand it, we might
> > have to devise a new structure to contain Oids and make ProcessUtility
> > accept a new nodeTag. We will also not be able to use existing Index
> > definition functions and this will lead to more coding IMHO. Do we want to
> > go down this path? Or is there something else that has been suggested above
> > and that I am missing completely?
>
>
> OTOH, we can populate a new structure with the relevant Oids, IndexInfo
> information from parent relation indexes and call index_create directly
> from within ProcessUtility. Guess, it should be cleaner than the current
> approach.
>
Sorry for the barrage of emails. But as I looked closely at the current
patch there are only 2 fields (accessMethod and tableSpace) in IndexStmt
structure that we populate by doing the conversion from OIDs to name. For
the other fields, the current transformations will remain.

If so, I think we can introduce 2 Oid fields in the IndexStmt structure and
store the Oids there. In DefineIndex we can use these Oids if they are not
invalid.

IMHO, all this is less work and the bulk of the changes remain localized in
mostly one or two functions as in the current patch.

Comments?

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS escribió:

> Sorry for the barrage of emails. But as I looked closely at the current
> patch there are only 2 fields (accessMethod and tableSpace) in IndexStmt
> structure that we populate by doing the conversion from OIDs to name. For
> the other fields, the current transformations will remain.
>
> If so, I think we can introduce 2 Oid fields in the IndexStmt structure and
> store the Oids there. In DefineIndex we can use these Oids if they are not
> invalid.

Sounds reasonable. This is what we do for example in VacuumStmt. Make
sure that the OIDs are set to Invalid in the parser.

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

NikhilS <nikkhils@gmail.com> writes:
> If so, I think we can introduce 2 Oid fields in the IndexStmt structure and
> store the Oids there. In DefineIndex we can use these Oids if they are not
> invalid.

I think this is just make-work that causes the patch to complicate parts
of the system it didn't need to touch. The original suggestion was to
actively refactor existing code, which might or might not have been
worthwhile. But this isn't an appropriate substitute --- it's just
making the API uglier for no particular benefit.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Hi,

On 5/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> NikhilS <nikkhils@gmail.com> writes:
> > If so, I think we can introduce 2 Oid fields in the IndexStmt structure
> and
> > store the Oids there. In DefineIndex we can use these Oids if they are
> not
> > invalid.
>
> I think this is just make-work that causes the patch to complicate parts
> of the system it didn't need to touch. The original suggestion was to
> actively refactor existing code, which might or might not have been
> worthwhile. But this isn't an appropriate substitute --- it's just
> making the API uglier for no particular benefit.


I agree this will unnecessary add arguments to the DefineIndex API. If we
stick to the patch's earlier way of converting the Oid to names for just
these 2 arguments, we can avoid this IMO.

Considering that we will be generating this information from existing valid
index information, I think converting the Oids to names is safe enough.
Alvaro, do you think we should stick to the existing patch mechanism then
considering that it avoids polluting the API?

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS escribió:

> On 5/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> >NikhilS <nikkhils@gmail.com> writes:
> >> If so, I think we can introduce 2 Oid fields in the IndexStmt
> >> structure and store the Oids there. In DefineIndex we can use these
> >> Oids if they are not invalid.
> >
> >I think this is just make-work that causes the patch to complicate parts
> >of the system it didn't need to touch. The original suggestion was to
> >actively refactor existing code, which might or might not have been
> >worthwhile. But this isn't an appropriate substitute --- it's just
> >making the API uglier for no particular benefit.
>
> I agree this will unnecessary add arguments to the DefineIndex API. If we
> stick to the patch's earlier way of converting the Oid to names for just
> these 2 arguments, we can avoid this IMO.
>
> Considering that we will be generating this information from existing valid
> index information, I think converting the Oids to names is safe enough.
> Alvaro, do you think we should stick to the existing patch mechanism then
> considering that it avoids polluting the API?

Not sure. Is it possible that the schema is renamed while the operation
is being executed? If it's not then this not a problem at all so the
existing patch is fine.

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

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Not sure. Is it possible that the schema is renamed while the operation
> is being executed? If it's not then this not a problem at all so the
> existing patch is fine.

There are hazards of that type in CREATE TABLE right now; it's hardly
fair to hold LIKE INCLUDING INDEXES to a higher standard.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match