Thread: Clear out the reminants of EXTEND INDEX
Wow Tom, I'm impressed. You took the entire index system and turned it on it's head, cleaned up most of my ugly bits and managed to remove most of the stuff relating to extend in the process. You missed a few spots though, so here a little patch to clear out the rest (although maybe you're doing them as we speak, can't tell). You also took care of my next idea, which was to allow IS NULL in the index predicate. Anyway, keep it up :) -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > It would be nice if someone came up with a certification system that > actually separated those who can barely regurgitate what they crammed over > the last few weeks from those who command secret ninja networking powers.
Attachment
> Wow Tom, I'm impressed. You took the entire index system and turned it on > it's head, cleaned up most of my ugly bits and managed to remove most of the > stuff relating to extend in the process. > > You missed a few spots though, so here a little patch to clear out the rest > (although maybe you're doing them as we speak, can't tell). > > You also took care of my next idea, which was to allow IS NULL in the index > predicate. > > Anyway, keep it up :) I am thinking I should apply this right away before the code changes again. Comments? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
> Wow Tom, I'm impressed. You took the entire index system and turned it on
> it's head, cleaned up most of my ugly bits and managed to remove most of the
> stuff relating to extend in the process.
>
> You missed a few spots though, so here a little patch to clear out the rest
> (although maybe you're doing them as we speak, can't tell).
>
> You also took care of my next idea, which was to allow IS NULL in the index
> predicate.
>
> Anyway, keep it up :)
> --
> Martijn van Oosterhout <kleptog@svana.org>
> http://svana.org/kleptog/
> > It would be nice if someone came up with a certification system that
> > actually separated those who can barely regurgitate what they crammed over
> > the last few weeks from those who command secret ninja networking powers.
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Martijn van Oosterhout <kleptog@svana.org> writes:
> You also took care of my next idea, which was to allow IS NULL in the index
> predicate.
No I didn't ... or at least, you can build such an index, but you can't
*do* much of anything useful with it. The indxpath.c
predicate-implication code needs work to understand IS NULL and friends
as well as plain operator expressions. Feel free to hack on it.
> diff -u -r1.91 nodes.h
> --- src/include/nodes/nodes.h 2001/06/19 22:39:12 1.91
> +++ src/include/nodes/nodes.h 2001/07/16 09:35:35
> @@ -158,7 +158,6 @@
> T_DropStmt,
> T_TruncateStmt,
> T_CommentStmt,
> - T_ExtendStmt,
> T_FetchStmt,
> T_IndexStmt,
> T_ProcedureStmt,
This is not good --- we try to avoid renumbering existing node types.
(Not for any essential reason, perhaps, but why risk breaking anything?)
My practice when removing a node type is to change the nodes.h entry
like this:
T_RemoveOperStmt,
T_RemoveStmt_XXX, /* not used anymore; tag# available */
T_RenameStmt,
Adding the _XXX is sufficient to ensure that any missed uses of the
typecode in the source will yield compile errors.
Otherwise it looks OK.
regards, tom lane
> > diff -u -r1.91 nodes.h > > --- src/include/nodes/nodes.h 2001/06/19 22:39:12 1.91 > > +++ src/include/nodes/nodes.h 2001/07/16 09:35:35 > > @@ -158,7 +158,6 @@ > > T_DropStmt, > > T_TruncateStmt, > > T_CommentStmt, > > - T_ExtendStmt, > > T_FetchStmt, > > T_IndexStmt, > > T_ProcedureStmt, > > This is not good --- we try to avoid renumbering existing node types. > (Not for any essential reason, perhaps, but why risk breaking anything?) > My practice when removing a node type is to change the nodes.h entry > like this: > > T_RemoveOperStmt, > T_RemoveStmt_XXX, /* not used anymore; tag# available */ > T_RenameStmt, > > Adding the _XXX is sufficient to ensure that any missed uses of the > typecode in the source will yield compile errors. > > Otherwise it looks OK. I always rip that stuff out. We do an initdb for every release. What could break by renumbering those nodes? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I always rip that stuff out. We do an initdb for every release. What
> could break by renumbering those nodes?
Actually, since we don't store node type numbers on disk (just symbolic
names), there's probably nothing that could break as long as people do a
full recompile when they update. It's no worse than changing the fields
of a widely-used struct, I suppose. I was just being paranoid.
If you want to apply the patch as-is then you might as well remove all
the "XXX"'d items in nodes.h...
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > If we do that, we have to only add entries to the end and not put them > > with other appropriate symbols, > > Au contraire: that's exactly why holes were left in the numbering to > start with. Yea, but in each group there are items that sort of go together. In the current XXX discussion, the Remove* items go together, right? If we had a new Remove item, we should put it with the others, not at the bottom. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I always rip that stuff out. We do an initdb for every release. What > > could break by renumbering those nodes? > > Actually, since we don't store node type numbers on disk (just symbolic > names), there's probably nothing that could break as long as people do a > full recompile when they update. It's no worse than changing the fields > of a widely-used struct, I suppose. I was just being paranoid. > > If you want to apply the patch as-is then you might as well remove all > the "XXX"'d items in nodes.h... OK, patch applied, and I applied the following additional patch to remove two XXX entries in nodes.h. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: nodes.h =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/nodes/nodes.h,v retrieving revision 1.92 diff -c -r1.92 nodes.h *** nodes.h 2001/07/16 19:07:40 1.92 --- nodes.h 2001/07/16 19:12:33 *************** *** 164,170 **** T_RemoveAggrStmt, T_RemoveFuncStmt, T_RemoveOperStmt, - T_RemoveStmt_XXX, /* not used anymore; tag# available */ T_RenameStmt, T_RuleStmt, T_NotifyStmt, --- 164,169 ---- *************** *** 221,227 **** T_BooleanTest, T_CaseExpr, T_CaseWhen, - T_RowMarkXXX, /* not used anymore; tag# available */ T_FkConstraint, T_PrivGrantee, --- 220,225 ----
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >>> I always rip that stuff out. We do an initdb for every release. What > >>> could break by renumbering those nodes? > >> > >> Actually, since we don't store node type numbers on disk (just symbolic > >> names), there's probably nothing that could break as long as people do a > >> full recompile when they update. It's no worse than changing the fields > >> of a widely-used struct, I suppose. I was just being paranoid. > > On third thought, there is some value to preserving the node numbers: > when we see a bug report from the field like > > ERROR: ExecEvalExpr: unknown expression type 108 > > we don't have to resort to excavating back versions of nodes.h to figure > out which node type is meant. If we do that, we have to only add entries to the end and not put them with other appropriate symbols, and we have to keep them around forever. Doesn't seem worth it to me. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>>> I always rip that stuff out. We do an initdb for every release. What
>>> could break by renumbering those nodes?
>>
>> Actually, since we don't store node type numbers on disk (just symbolic
>> names), there's probably nothing that could break as long as people do a
>> full recompile when they update. It's no worse than changing the fields
>> of a widely-used struct, I suppose. I was just being paranoid.
On third thought, there is some value to preserving the node numbers:
when we see a bug report from the field like
ERROR: ExecEvalExpr: unknown expression type 108
we don't have to resort to excavating back versions of nodes.h to figure
out which node type is meant.
regards, tom lane
Patch applied. > Wow Tom, I'm impressed. You took the entire index system and turned it on > it's head, cleaned up most of my ugly bits and managed to remove most of the > stuff relating to extend in the process. > > You missed a few spots though, so here a little patch to clear out the rest > (although maybe you're doing them as we speak, can't tell). > > You also took care of my next idea, which was to allow IS NULL in the index > predicate. > > Anyway, keep it up :) > -- > Martijn van Oosterhout <kleptog@svana.org> > http://svana.org/kleptog/ > > It would be nice if someone came up with a certification system that > > actually separated those who can barely regurgitate what they crammed over > > the last few weeks from those who command secret ninja networking powers. [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Yea, but in each group there are items that sort of go together. In the
> current XXX discussion, the Remove* items go together, right? If we had
> a new Remove item, we should put it with the others, not at the bottom.
That has not been our practice in the past. For the most part we've
added new node types at the ends of their sections. If we were going
to organize the numbering as you suggest then a lot of rearrangement
could be done (and NO, I am NOT advocating that).
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> If we do that, we have to only add entries to the end and not put them
> with other appropriate symbols,
Au contraire: that's exactly why holes were left in the numbering to
start with.
regards, tom lane