Thread: Outstanding patches
OK, now that we have started 7.2 development, I am going to go through
the outstanding patches and start to apply them or reject them. They
are at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I could use help in identifying which patches are a problem. Most of
the ones there now have been reviewed by me or have received the
recommendation of another developer.
I particularly need JDBC help because I have many JDBC patches.
If you would send email stating which patches should _not_ be applied, I
would appreciate it.
Of course, patches can always be backed out.
--
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: > OK, now that we have started 7.2 development, I am going to go through > the outstanding patches and start to apply them or reject them. They > are at: > http://candle.pha.pa.us/cgi-bin/pgpatches > I could use help in identifying which patches are a problem. Most of > the ones there now have been reviewed by me or have received the > recommendation of another developer. Okay, I looked through these ... I do not like Ian Taylor's plpgsql cursor patch; trying to do cursors inside plpgsql with no SPI-level support is too much of a kluge. We should first add cursor support to SPI, then fix plpgsql. Much of the parsing work he's done could be salvaged, but the implementation can't be. (And I don't want to apply it now and back it out later --- it adds too many warts.) Fernando Nasser's ANALYZE patch is superseded by already-applied work, though if he wants to do the promised test additions I would be happy. The PAM support patch concerns me --- it looks like yet another chunk of code that will tie up the postmaster in a single-threaded conversation with a remote daemon that may or may not respond promptly. I recommend holding off on this until we think about whether we shouldn't restructure the postmaster to do all authentication work in per-client subprocesses. We need to discuss whether we like the %TYPE feature proposed by Ian Taylor. It seems awfully nonstandard to me, and I'm not sure that the value is great enough to be worth inventing a nonstandard feature. ISTM that people don't normally tie functions to tables so tightly that it's better to define a function in terms of "the type of column foo of table bar" than just in terms of the type itself. Ian claims that this is helpful, but is it really likely that you can change that column type without making *any* other mods to the function? Moreover, in exchange for this possible benefit you are opening yourself to breaking the function if you choose to rename either the column or the table. The potential net gain seems really small. (If we do like the functionality, then the patch itself seems OK with the exception of the gram.y definition of func_type; the table name should be TokenId not just IDENT.) I did not look at any of the JDBC or libpq++ patches. The other stuff seemed OK on a first glance. regards, tom lane
> Okay, I looked through these ... Thanks. > I do not like Ian Taylor's plpgsql cursor patch; trying to do cursors > inside plpgsql with no SPI-level support is too much of a kluge. We > should first add cursor support to SPI, then fix plpgsql. Much of the > parsing work he's done could be salvaged, but the implementation can't > be. (And I don't want to apply it now and back it out later --- it adds > too many warts.) I know Jan is talking about SPI support fpr plpgsql. I will keep the patch but not apply it. > Fernando Nasser's ANALYZE patch is superseded by already-applied work, > though if he wants to do the promised test additions I would be happy. I have already emailed him to say you did it already. Removed. > The PAM support patch concerns me --- it looks like yet another chunk > of code that will tie up the postmaster in a single-threaded > conversation with a remote daemon that may or may not respond promptly. > I recommend holding off on this until we think about whether we > shouldn't restructure the postmaster to do all authentication work in > per-client subprocesses. I have not idea what PAM is. If it is a valuable feature, we can install it. But if it is yet another authentication scheme, it could add more confusion to our already complicated setup. Seems you are saying it is the latter, which is fine with me. > We need to discuss whether we like the %TYPE feature proposed by Ian > Taylor. It seems awfully nonstandard to me, and I'm not sure that the > value is great enough to be worth inventing a nonstandard feature. > ISTM that people don't normally tie functions to tables so tightly that > it's better to define a function in terms of "the type of column foo > of table bar" than just in terms of the type itself. Ian claims that > this is helpful, but is it really likely that you can change that column > type without making *any* other mods to the function? Moreover, in > exchange for this possible benefit you are opening yourself to breaking > the function if you choose to rename either the column or the table. > The potential net gain seems really small. (If we do like the > functionality, then the patch itself seems OK with the exception of the > gram.y definition of func_type; the table name should be TokenId not > just IDENT.) I thought it was valuable. I know in Informix 4gl you can set variables to track column types, and it helps, especially when you make a column longer or something. It also better documents the variable. I remember someone else stating this was a nice feature, so I am inclinded to apply it, with your suggested changes. > I did not look at any of the JDBC or libpq++ patches. The other stuff > seemed OK on a first glance. Ditto. JDBC will need comment from JDBC people. The libpq++ stuff looks pretty good 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, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> We need to discuss whether we like the %TYPE feature proposed by Ian
>> Taylor. It seems awfully nonstandard to me, and I'm not sure that the
>> value is great enough to be worth inventing a nonstandard feature.
>> ISTM that people don't normally tie functions to tables so tightly that
>> it's better to define a function in terms of "the type of column foo
>> of table bar" than just in terms of the type itself. Ian claims that
>> this is helpful, but is it really likely that you can change that column
>> type without making *any* other mods to the function? Moreover, in
>> exchange for this possible benefit you are opening yourself to breaking
>> the function if you choose to rename either the column or the table.
>> The potential net gain seems really small. (If we do like the
>> functionality, then the patch itself seems OK with the exception of the
>> gram.y definition of func_type; the table name should be TokenId not
>> just IDENT.)
> I thought it was valuable. I know in Informix 4gl you can set variables
> to track column types, and it helps, especially when you make a column
> longer or something. It also better documents the variable.
But it's not really tracking the variable; with Ian's proposed
implementation, after
create table foo(bar int4);
create function fooey(foo.bar%type) ...;
drop table foo;
create table foo(bar int8);
you would still have fooey declared as taking int4 not int8, because
the type meant by %type is resolved and frozen immediately upon being
seen.
Moreover, because of our function-name-overloading feature, fooey(int4)
and fooey(int8) are two different functions. IMHO it would be a bad
thing if we *did* try to change the signature. We'd break existing
callers of the function, not to mention possibly run into a naming
conflict with a pre-existing fooey(int8).
I presume that Ian is not thinking about such a scenario, but only about
using %type in a schema file that he will reload into a freshly created
database each time he edits it. That avoids the issue of whether %type
declarations can or should track changes on the fly, but I think he's
still going to run into problems with function naming: do
fooey(foo.bar%type) and fooey(foo.baz%type) conflict, or not? Maybe
today the schema works and tomorrow you get an error.
Basically I think that this feature does not coexist well with function
overloading, and that it's likely to create as much or more grief as it
avoids.
regards, tom lane
> I presume that Ian is not thinking about such a scenario, but only about > using %type in a schema file that he will reload into a freshly created > database each time he edits it. That avoids the issue of whether %type > declarations can or should track changes on the fly, but I think he's > still going to run into problems with function naming: do > fooey(foo.bar%type) and fooey(foo.baz%type) conflict, or not? Maybe > today the schema works and tomorrow you get an error. > > Basically I think that this feature does not coexist well with function > overloading, and that it's likely to create as much or more grief as it > avoids. But don't we already have problems with changing functions that use tables or does this open a new type of problem? Seems if you define a function to be based on a column, and the column changes, the person should expect errors. If we define things as %TYPE in plpgsql, do we handle cases when the column type changes? -- 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, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> But don't we already have problems with changing functions that use
> tables or does this open a new type of problem?
But this feature isn't about functions that use tables internally;
it's about tying the fundamental signature of the function to a table.
I doubt that that's a good idea. It definitely does introduce potential
for problems that weren't there before, per the illustrations I already
gave.
You commented earlier that it's easy to "change the width of a column"
with this approach, but that's irrelevant, because atttypmod is not part
of a function's signature anyhow.
> If we define things as %TYPE in plpgsql, do we handle cases when the
> column type changes?
Sort of, because we just need to drop the cached precompiled version of
the function --- you can do that by starting a fresh backend if nothing
else, and we have speculated about making it happen automatically.
Changing a function's signature automatically is a MUCH bigger and
nastier can of worms, because it affects things outside the function.
regards, tom lane
On Tue, May 08, 2001 at 05:49:16PM -0400, Tom Lane wrote: > I presume that Ian is not thinking about such a scenario, but only about > using %type in a schema file that he will reload into a freshly created > database each time he edits it. That avoids the issue of whether %type > declarations can or should track changes on the fly, but I think he's > still going to run into problems with function naming: do > fooey(foo.bar%type) and fooey(foo.baz%type) conflict, or not? Maybe > today the schema works and tomorrow you get an error. How about a feature in psql which would read something like '%type' and convert it to the appropriate thing before it passed it to the backend? Then you could use it without thinking about it in a script which you would \i into psql. That would do what's wanted here without having any backend nasties. I'm not offering to implement it myself - at least not right now - but does it seem like a sensible idea? Richard
Richard Poole <richard.poole@vi.net> writes:
> How about a feature in psql which would read something like '%type' and
> convert it to the appropriate thing before it passed it to the backend?
That's just about what Ian's patch does, only it does it during backend
parsing instead of in the client. It seems to me that most of the
arguments against it apply either way.
If we are going to have it, the backend is certainly the right place to
do it, rather than adding a huge amount of new smarts to psql.
regards, tom lane
> But this feature isn't about functions that use tables internally; > it's about tying the fundamental signature of the function to a table. > I doubt that that's a good idea. It definitely does introduce potential > for problems that weren't there before, per the illustrations I already > gave. > > You commented earlier that it's easy to "change the width of a column" > with this approach, but that's irrelevant, because atttypmod is not part > of a function's signature anyhow. Yea, that is more an Informix issue. > > If we define things as %TYPE in plpgsql, do we handle cases when the > > column type changes? > > Sort of, because we just need to drop the cached precompiled version of > the function --- you can do that by starting a fresh backend if nothing > else, and we have speculated about making it happen automatically. > Changing a function's signature automatically is a MUCH bigger and > nastier can of worms, because it affects things outside the function. OK, one idea is to throw a elog(NOTICE) when they use this feature, stating that it will not track column changes. Another option is to just forget about the feature entirely. Do we have people who like this feature? Speak up now. If not, we will drop it. -- 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, Pennsylvania19026
Tom Lane <tgl@sss.pgh.pa.us> writes: > I do not like Ian Taylor's plpgsql cursor patch; trying to do cursors > inside plpgsql with no SPI-level support is too much of a kluge. We > should first add cursor support to SPI, then fix plpgsql. Much of the > parsing work he's done could be salvaged, but the implementation can't > be. (And I don't want to apply it now and back it out later --- it adds > too many warts.) I think most of the cursor patch will stand even after SPI-level support for cursors is added. But it's up to you, of course. 7.2 is a long way away in any case. I would be happy to rework the patch when SPI supports cursors. > We need to discuss whether we like the %TYPE feature proposed by Ian > Taylor. It seems awfully nonstandard to me, and I'm not sure that the > value is great enough to be worth inventing a nonstandard feature. Oracle PL/SQL supports this, and PL/SQL code that I've seen uses it extensively. PL/pgSQL supports %TYPE in all places a type may be used, except parameter and return types. > ISTM that people don't normally tie functions to tables so tightly that > it's better to define a function in terms of "the type of column foo > of table bar" than just in terms of the type itself. Ian claims that > this is helpful, but is it really likely that you can change that column > type without making *any* other mods to the function? Sure. I've seen code in which all access to the database is done via stored procedures. It's natural to write that sort of code using %TYPE. Otherwise any change you make to the schema, you have to make two or three times. Admittedly, this may apply mostly to what Postgres calls type modifiers. But it's still a natural way to write the procedure. Why duplicate information? > Moreover, in > exchange for this possible benefit you are opening yourself to breaking > the function if you choose to rename either the column or the table. If you do that you've most likely broken the function anyhow, since you probably wouldn't use %TYPE if you weren't referring to the column. Anyhow, if you don't use %TYPE you can break the function in the other way, by changing the type of the column. So I think it's six of one, half-dozen of the other. > (If we do like the > functionality, then the patch itself seems OK with the exception of the > gram.y definition of func_type; the table name should be TokenId not > just IDENT.) I think I tried that, and I think it led to lots of reduce/reduce errors. But maybe that was only in 7.0.3. The problem that the function type does not change when the schema changes is problematical. I would have been happier if I could have left the %TYPE as a string to be interpreted at execution time. But of course that does not work with the current system for function overloading. Ian ---------------------------(end of broadcast)--------------------------- TIP 483: In Lowes Crossroads, Delaware, it is a violation of local law for any pilot or passenger to carry an ice cream cone in their pocket while either flying or waiting to board a plane.
Tom Lane wrote: > But it's not really tracking the variable; with Ian's proposed > implementation, after > > create table foo(bar int4); > > create function fooey(foo.bar%type) ...; > > drop table foo; > > create table foo(bar int8); > > you would still have fooey declared as taking int4 not int8, because > the type meant by %type is resolved and frozen immediately upon being > seen. Ok, this is a more general point: in Oracle (which, as Ian points out, uses this feature extensively) if you recreate table foo, function fooey is tagged as 'dirty' and recompiled on the spot next time is used. This is also true for VIEWs and other objects, so you don't have the problem we have when a view breaks because you've updated the underlining table. -- Alessio F. Bragadini alessio@albourne.com APL Financial Services http://village.albourne.com Nicosia, Cyprus phone: +357-2-755750 "It is more complicated than you think" -- The Eighth Networking Truth from RFC 1925
Is anybody planning to fix the problem with ALTER TABLE ADD CONSTRAINT... in which the constraints are not applied to child tables? ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
On Tue, 8 May 2001, Bruce Momjian wrote: > > The PAM support patch concerns me --- it looks like yet another chunk > > of code that will tie up the postmaster in a single-threaded > > conversation with a remote daemon that may or may not respond promptly. > > I recommend holding off on this until we think about whether we > > shouldn't restructure the postmaster to do all authentication work in > > per-client subprocesses. > > I have not idea what PAM is. If it is a valuable feature, we can > install it. But if it is yet another authentication scheme, it could > add more confusion to our already complicated setup. Seems you are > saying it is the latter, which is fine with me. PAM is a universal interface to many authentication schemes. If PostgreSQL supports PAM properly, it can instantly support many different types of authentication, such as UNIX, Kerberos, RADIUS, LDAP, or even Windows NT domain authentication. Solaris and most modern Linux distributions (certainly Red Hat) support PAM: http://www.sun.com/solaris/pam/ http://www.kernel.org/pub/linux/libs/pam/ PAM modules are very flexible -- they are even stackable. I've used PAM to allow the UW IMAP server running on Red Hat Linux to get its passwords either from UNIX authentication or from a Windows NT server, for example. Given that this has the potential to reduce the number of places that system administrators have to maintain passwords, I'd call it a win overall, except for that pesky single-threaded issue. You should keep in mind, though, that some PAM calls won't involve calls to daemons that might not be responsive. Let's say PAM is configured to check UNIX authentication (/etc/passwd and /etc/shadow) for passwords -- there is no daemon involved, just calls to C libraries that will return promptly. If the PAM config file had something like LDAP authentication indicated, you would have a potential issue if the LDAP server did not respond. As long as this limitation was documented, though, this would be a very valuable addition. A release note saying that the feature was experimental, and outlining the limitations in the face of choosing an authentication scheme that may fail to answer might be appropriate. --Richard Bullington-McGuire <rbulling@microstate.com>Chief Technology Officer, The Microstate CorporationPhone: 703-796-6446 URL: http://www.microstate.com/PGP key IDs: RSA: 0x93862305 DH/DSS: 0xDAC3028E
> > Is anybody planning to fix the problem with ALTER TABLE ADD CONSTRAINT... > in which the constraints are not applied to child tables? I thought we had not figured out how to inherit those, or at least certain constraints like UNIQUE. We do have on TODO: * Allow inherited tables to inherit index, UNIQUE constraint, andprimary key [inheritance] -- 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, Pennsylvania19026
At 09:36 9/05/01 -0400, Bruce Momjian wrote: >> >> Is anybody planning to fix the problem with ALTER TABLE ADD CONSTRAINT... >> in which the constraints are not applied to child tables? > >I thought we had not figured out how to inherit those, or at least >certain constraints like UNIQUE. We do have on TODO: > > * Allow inherited tables to inherit index, UNIQUE constraint, and > primary key [inheritance] > aaa=# create table t1(f1 integer check(f1<>0),primary key (f1)); aaa=# create table t1c() inherits (t1); aaa=# \d t1c Table "t1c"Attribute | Type | Modifier -----------+---------+----------f1 | integer | not null Constraint: (f1 <> 0) So PK is not inherited, but CHECK (and implied NOT NULL) seem to be. Whereas, aaa=# create table t1(f1 integer); aaa=# create table t1c() inherits (t1); aaa=# alter table t1 add constraint aaa check(f1<>0); aaa=# \d t1c Table "t1c"Attribute | Type | Modifier -----------+---------+----------f1 | integer | ie. The CHECK constraints inherit only at the time of table creation. I think this is a bug in ALTER TABLE for CHECK constraints. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
Alessio Bragadini <alessio@albourne.com> writes:
> Tom Lane wrote:
>> But it's not really tracking the variable; with Ian's proposed
>> implementation, after
>>
>> create table foo(bar int4);
>>
>> create function fooey(foo.bar%type) ...;
>>
>> drop table foo;
>>
>> create table foo(bar int8);
>>
>> you would still have fooey declared as taking int4 not int8, because
>> the type meant by %type is resolved and frozen immediately upon being
>> seen.
> Ok, this is a more general point: in Oracle (which, as Ian points out,
> uses this feature extensively) if you recreate table foo, function fooey
> is tagged as 'dirty' and recompiled on the spot next time is used. This
> is also true for VIEWs and other objects, so you don't have the problem
> we have when a view breaks because you've updated the underlining table.
Indeed, and we have plans to do something similar sometime soon. My
real objection to this proposed feature is that there is no way to
handle the update as a local matter within the function, because
changing the function's input datatypes actually means it's a different
function. This creates all sorts of problems at both the definitional
and implementation levels...
regards, tom lane
Philip Warner <pjw@rhyme.com.au> writes:
> Is anybody planning to fix the problem with ALTER TABLE ADD CONSTRAINT...
> in which the constraints are not applied to child tables?
AFAIK no one is looking at it presently (although Stephan Szabo has
probably thought about it). If you want to tackle it, step right up,
but coordinate with Stephan.
I was just in the vicinity of ALTER TABLE, and noted that that routine
didn't have the same loop-over-children superstructure that most of the
other ALTER code does. Should be a relatively simple matter to graft
that logic onto it, unless there are semantic funnies that come up with
propagating the new constraint.
regards, tom lane
Philip Warner <pjw@rhyme.com.au> writes:
> ie. The CHECK constraints inherit only at the time of table creation. I
> think this is a bug in ALTER TABLE for CHECK constraints.
More like an "unimplemented feature" ;-).
After thinking for a moment, I believe the only real gotcha that could
arise here is to make sure that the constraint is adjusted for the
possibly-different column numbers in each child table. There is code
available to make this happen, or it might happen for free if you can
postpone parse analysis of the raw constraint tree until you are looking
at each child table. Just something to keep in mind and test while
you're doing it ...
regards, tom lane
> > Ok, this is a more general point: in Oracle (which, as Ian points out, > > uses this feature extensively) if you recreate table foo, function fooey > > is tagged as 'dirty' and recompiled on the spot next time is used. This > > is also true for VIEWs and other objects, so you don't have the problem > > we have when a view breaks because you've updated the underlining table. > > Indeed, and we have plans to do something similar sometime soon. My > real objection to this proposed feature is that there is no way to > handle the update as a local matter within the function, because > changing the function's input datatypes actually means it's a different > function. This creates all sorts of problems at both the definitional > and implementation levels... Does this relate to allowing functions to be recreated with the same OID as the original function? I think we need that badly for 7.2. -- 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, Pennsylvania19026
On Wed, 9 May 2001, Philip Warner wrote: > > Is anybody planning to fix the problem with ALTER TABLE ADD CONSTRAINT... > in which the constraints are not applied to child tables? > I'm working on the check constraint case (didn't realize that those inherited since unique, primary key and foreign key can't right now). We need to really figure out what we're going to do with the other constraints to make them inherit.
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Does this relate to allowing functions to be recreated with the same OID > > as the original function? I think we need that badly for 7.2. > > No, I don't think that's very related; that's a simple matter of > implementing an ALTER FUNCTION command. The other thing will require > figuring out how to do dependency tracking. Got it. Let me ask, if they change the column type, would they use ALTER FUNCTION to then update to match the new column type. As I understand it, the problem is that this does not happen automatically, right? -- 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, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Does this relate to allowing functions to be recreated with the same OID
> as the original function? I think we need that badly for 7.2.
No, I don't think that's very related; that's a simple matter of
implementing an ALTER FUNCTION command. The other thing will require
figuring out how to do dependency tracking.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> No, I don't think that's very related; that's a simple matter of
>> implementing an ALTER FUNCTION command. The other thing will require
>> figuring out how to do dependency tracking.
> Got it. Let me ask, if they change the column type, would they use
> ALTER FUNCTION to then update to match the new column type. As I
> understand it, the problem is that this does not happen automatically,
> right?
My vision of ALTER FUNCTION is that it would let you change the function
body, and perhaps also the function language and attributes (isCachable,
isStrict). It would NOT allow you to change the function's parameter
types or return type, because that potentially breaks things that depend
on the function. To do that, you should have to create a new function.
regards, tom lane
Bruce Momjian writes: > I could use help in identifying which patches are a problem. Most of > the ones there now have been reviewed by me or have received the > recommendation of another developer. I have a few stylistic issues with the createlang patch, but I can work on installing it myself. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
I'll have a look-see since it's in the vicinity of the code I'm messing with at the moment. Time is my problem, though... Chris > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org]On Behalf Of Tom Lane > Sent: Wednesday, 9 May 2001 11:00 PM > To: Philip Warner > Cc: PostgreSQL-development > Subject: Re: [HACKERS] Outstanding patches > > > Philip Warner <pjw@rhyme.com.au> writes: > > Is anybody planning to fix the problem with ALTER TABLE ADD > CONSTRAINT... > > in which the constraints are not applied to child tables? > > AFAIK no one is looking at it presently (although Stephan Szabo has > probably thought about it). If you want to tackle it, step right up, > but coordinate with Stephan. > > I was just in the vicinity of ALTER TABLE, and noted that that routine > didn't have the same loop-over-children superstructure that most of the > other ALTER code does. Should be a relatively simple matter to graft > that logic onto it, unless there are semantic funnies that come up with > propagating the new constraint. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) >
Bruce Momjian wrote: > > OK, now that we have started 7.2 development, I am going to go through > the outstanding patches and start to apply them or reject them. They > are at: > > http://candle.pha.pa.us/cgi-bin/pgpatches > Has the patch that makes MOVE return number of rows actually moved (analoguous to UPDATE and DELETE) been properly submitted to patches ? -------------------- Hannu
[ Charset ISO-8859-15 unsupported, converting... ] > Bruce Momjian wrote: > > > > OK, now that we have started 7.2 development, I am going to go through > > the outstanding patches and start to apply them or reject them. They > > are at: > > > > http://candle.pha.pa.us/cgi-bin/pgpatches > > > > Has the patch that makes MOVE return number of rows actually moved > (analoguous > to UPDATE and DELETE) been properly submitted to patches ? I know MOVE had fixes in 7.1. I don't know of any outstanding MOVE bugs. -- 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:
>> Has the patch that makes MOVE return number of rows actually moved
>> (analoguous to UPDATE and DELETE) been properly submitted to patches ?
> I know MOVE had fixes in 7.1. I don't know of any outstanding MOVE
> bugs.
It wasn't a bug, it was a feature ;-)
Bruce did not have that patch on his list of things-to-apply, so either
it was never properly submitted or it slipped through the cracks.
Anyone want to dig it up and verify it against 7.1?
regards, tom lane
Tom Lane wrote:
>
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> Has the patch that makes MOVE return number of rows actually moved
> >> (analoguous to UPDATE and DELETE) been properly submitted to patches ?
>
> > I know MOVE had fixes in 7.1. I don't know of any outstanding MOVE
> > bugs.
>
> It wasn't a bug, it was a feature ;-)
>
> Bruce did not have that patch on his list of things-to-apply, so either
> it was never properly submitted or it slipped through the cracks.
> Anyone want to dig it up and verify it against 7.1?
I forward it here so you don't have to dig it up:
-----------------------------------------------------------------------
Hi.
A few weeks (months?) ago I made a patch to the postgres
backend to get back the number of realized moves after
a MOVE command. So if I issue a "MOVE 100 IN cusrorname",
but there was only 66 rows left, I get back not only "MOVE",
but "MOVE 66". If the 100 steps could be realized, then
"MOVE 100" would come back.
I send this info to you, because I would like to ask you if
it could be OK to include in future versions. I think you
are in a beta testing phase now, so it is trivially not the
time to include it now...
The solution is 2 code lines into command.c, and then the
message of move cames with the number into for example psql.
1 other word into the jdbc driver, and this number is
available at update_count.
I made the patch to the latest (one day old) CVS version.
Here are the patches. Please look at them, and if you think
it's a good idea, then please let me know where and how should
I post them, and approximately when will you finish with the
beta testing, so it can be really considered seriously.
I included them also as an attachment, because my silly pine
insists to break the lines...
--- ./src/backend/commands/command.c.orig Fri Mar 23 05:49:52 2001
+++ ./src/backend/commands/command.c Sat Apr 7 10:24:27 2001
@@ -174,6 +174,12 @@
if (!portal->atEnd)
{
ExecutorRun(queryDesc, estate, EXEC_FOR, (long) count);
+
+ /* I use CMD_UPDATE, because no CMD_MOVE or the like
+ exists, and I would like to provide the same
+ kind of info as CMD_UPDATE */
+ UpdateCommandInfo(CMD_UPDATE, 0, estate->es_processed);
+
if (estate->es_processed > 0)
portal->atStart = false; /* OK to back up now */
if (count <= 0 || (int) estate->es_processed < count)
@@ -185,6 +191,12 @@
if (!portal->atStart)
{
ExecutorRun(queryDesc, estate, EXEC_BACK, (long) count);
+
+ /* I use CMD_UPDATE, because no CMD_MOVE or the like
+ exists, and I would like to provide the same
+ kind of info as CMD_UPDATE */
+ UpdateCommandInfo(CMD_UPDATE, 0, -1*estate->es_processed);
+
if (estate->es_processed > 0)
portal->atEnd = false; /* OK to go forward now */
if (count <= 0 || (int) estate->es_processed < count)
Here is the patch for the jdbc driver. >! I couldn't test it
with the current version, because it needs ant, and I didn't
have time and money today to download it... !< However, it
is a trivial change, and if Peter T. Mount reads it, I ask
him to check if he likes it... Thanks for any kind of answer.
--- ./src/interfaces/jdbc/org/postgresql/Connection.java.orig Wed Jan 31
09:26:01 2001
+++ ./src/interfaces/jdbc/org/postgresql/Connection.java Sat Apr 7
16:42:04 2001
@@ -490,7 +490,7 @@
recv_status =
pg_stream.ReceiveString(receive_sbuf,8192,getEncoding());
// Now handle the update count correctly.
- if(recv_status.startsWith("INSERT") ||
recv_status.startsWith("UPDATE") || recv_status.startsWith("DELETE")) {
+ if(recv_status.startsWith("INSERT") ||
recv_status.startsWith("UPDATE") || recv_status.startsWith("DELETE") ||
recv_status.startsWith("MOVE")) {
try {
update_count =
Integer.parseInt(recv_status.substring(1+recv_status.lastIndexOf(' ')));
} catch(NumberFormatException nfe) {
-------------------
(This last looks a bit complex, but the change is really a new
"|| recv_status.startsWith("MOVE")" only...)
Thank you for having read this,
Baldvin
p.s.: I read a page on your homepage, called "unapplied patches".
I would like to know if it means "still unapplied patches", or
it means "bad, and not accepted ideas".
> Here are the patches. Please look at them, and if you think > it's a good idea, then please let me know where and how should > I post them, and approximately when will you finish with the > beta testing, so it can be really considered seriously. > > I included them also as an attachment, because my silly pine > insists to break the lines... Looks fine to me. I don't remember ever seeing this before. > p.s.: I read a page on your homepage, called "unapplied patches". > I would like to know if it means "still unapplied patches", or > it means "bad, and not accepted ideas". It means it is probably good, waiting for approval from others, or in the standard one-delay before applying patches. -- 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
> + /* I use CMD_UPDATE, because no CMD_MOVE or the like
> + exists, and I would like to provide the same
> + kind of info as CMD_UPDATE */
> + UpdateCommandInfo(CMD_UPDATE, 0, -1*estate->es_processed);
I do not think it is a good idea to return a negative count for a
backwards move; that is too likely to break client code that parses
command result strings and isn't expecting minus signs. The client
should know whether he issued MOVE FORWARD or MOVE BACKWARDS anyway,
so just returning es_processed ought to be sufficient.
Otherwise I think the patch is probably OK.
regards, tom lane
[ Charset ISO-8859-15 unsupported, converting... ] > Bruce Momjian wrote: > > > > OK, now that we have started 7.2 development, I am going to go through > > the outstanding patches and start to apply them or reject them. They > > are at: > > > > http://candle.pha.pa.us/cgi-bin/pgpatches > > > > Has the patch that makes MOVE return number of rows actually moved > (analoguous > to UPDATE and DELETE) been properly submitted to patches ? Yes, it is on the page to be applied, but the page also has Tom Lane's objection to returning a negative value. Can you fix that and resubmit? -- 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
Ian Lance Taylor wrote: > > Oracle PL/SQL supports this, and PL/SQL code that I've seen uses it > extensively. PL/pgSQL supports %TYPE in all places a type may be > used, except parameter and return types. It's not PL/pgSQL's fault here. The pg_proc entries are created by the CREATE FUNCTION utility command that's used for all languages. So what we're talking about affects SQL, C, PL/Tcl, PL/Perl, PL/Python and whatnot too.PL/pgSQL might live with that very well, because it has some automatic type conversion (using the actual valuestypoutput and the needed types typinput functions) to convert values on the fly. But a C function receivinga different type all of a sudden is a good candidate to coredump the backend. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com # _________________________________________________________ Do You Yahoo!? Get your free @yahoo.com address at http://mail.yahoo.com
Can this patch be resubmitted with a postive-only return value? > > + /* I use CMD_UPDATE, because no CMD_MOVE or the like > > + exists, and I would like to provide the same > > + kind of info as CMD_UPDATE */ > > + UpdateCommandInfo(CMD_UPDATE, 0, -1*estate->es_processed); > > I do not think it is a good idea to return a negative count for a > backwards move; that is too likely to break client code that parses > command result strings and isn't expecting minus signs. The client > should know whether he issued MOVE FORWARD or MOVE BACKWARDS anyway, > so just returning es_processed ought to be sufficient. > > Otherwise I think the patch is probably OK. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- 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
> > + /* I use CMD_UPDATE, because no CMD_MOVE or the like > > + exists, and I would like to provide the same > > + kind of info as CMD_UPDATE */ > > + UpdateCommandInfo(CMD_UPDATE, 0, -1*estate->es_processed); > > I do not think it is a good idea to return a negative count for a > backwards move; that is too likely to break client code that parses > command result strings and isn't expecting minus signs. The client > should know whether he issued MOVE FORWARD or MOVE BACKWARDS anyway, > so just returning es_processed ought to be sufficient. > > Otherwise I think the patch is probably OK. I have applied this patch with does MOVE output for both the backend and jdbc. I tested the JDBC patch by compiling, and changed the backend to only output postitive values. Thanks. -- 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: src/backend/commands/command.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/commands/command.c,v retrieving revision 1.131 diff -c -r1.131 command.c *** src/backend/commands/command.c 2001/05/30 13:00:03 1.131 --- src/backend/commands/command.c 2001/06/07 00:03:44 *************** *** 176,181 **** --- 176,187 ---- if (!portal->atEnd) { ExecutorRun(queryDesc, estate, EXEC_FOR, (long) count); + /* + * I use CMD_UPDATE, because no CMD_MOVE or the like + * exists, and I would like to provide the same + * kind of info as CMD_UPDATE + */ + UpdateCommandInfo(CMD_UPDATE, 0, estate->es_processed); if (estate->es_processed > 0) portal->atStart = false; /* OK to back up now */ if (count <= 0 || (int) estate->es_processed < count) *************** *** 187,192 **** --- 193,204 ---- if (!portal->atStart) { ExecutorRun(queryDesc, estate, EXEC_BACK, (long) count); + /* + * I use CMD_UPDATE, because no CMD_MOVE or the like + * exists, and I would like to provide the same + * kind of info as CMD_UPDATE + */ + UpdateCommandInfo(CMD_UPDATE, 0, estate->es_processed); if (estate->es_processed > 0) portal->atEnd = false; /* OK to go forward now */ if (count <= 0 || (int) estate->es_processed < count) Index: src/interfaces/jdbc/org/postgresql/Connection.java =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/jdbc/org/postgresql/Connection.java,v retrieving revision 1.16 diff -c -r1.16 Connection.java *** src/interfaces/jdbc/org/postgresql/Connection.java 2001/06/01 20:57:58 1.16 --- src/interfaces/jdbc/org/postgresql/Connection.java 2001/06/07 00:03:56 *************** *** 505,511 **** recv_status = pg_stream.ReceiveString(receive_sbuf,8192,getEncoding()); // Now handle the update count correctly. ! if(recv_status.startsWith("INSERT") || recv_status.startsWith("UPDATE") || recv_status.startsWith("DELETE")){ try { update_count = Integer.parseInt(recv_status.substring(1+recv_status.lastIndexOf(' '))); } catch(NumberFormatException nfe) { --- 505,511 ---- recv_status = pg_stream.ReceiveString(receive_sbuf,8192,getEncoding()); // Now handle the update count correctly. ! if(recv_status.startsWith("INSERT") || recv_status.startsWith("UPDATE") || recv_status.startsWith("DELETE")|| recv_status.startsWith("MOVE")) { try { update_count = Integer.parseInt(recv_status.substring(1+recv_status.lastIndexOf(' '))); } catch(NumberFormatException nfe) {