Re: [PATCHES] [HACKERS] Patch Submission Guidelines
Robert Treat <[EMAIL PROTECTED]> writes: > As stated, the following patch adds a list of patch submission guidelines > based on Simon Riggs suggestions to the developers FAQ. A couple minor comments ... > ! Ensure that your patch is generated against the most recent > version > ! of the code. If you are developing new features, this should be > ! CVS HEAD; if it is a bug fix, this will be the most recent > version of > ! the branch which suffers from the bug. For more on branches in > ! PostgreSQL, see 1.15. Actually, I'd suggest working against HEAD in all cases; the committers are used to adapting patches backwards, less so to adapting forwards. (If a bug is fixed in newer releases and not older ones, there is probably a good reason why not; so I don't see the point of encouraging people to submit patches for bugs that only exist in older releases, as this text seems to do.) > ! The patch should be generated in contextual diff format and > should > ! be applicable from the root directory. If you are unfamiliar > with > ! this, you might find the script > src/tools/makediff/difforig > ! useful. Unified diffs are only preferrable if the file changes > are > ! single-line changes and do not rely on the surrounding > lines. I'd like the policy to be "contextual diffs are preferred", full stop. Unidiffs are more compact but they sacrifice readability of the patch (IMHO anyway) and when you are preparing a patch you should be thinking first in terms of making it readable for the reviewers/committers. Some things that follow along with the readability mandate, and should be brought out somewhere here: * avoid unnecessary whitespace changes. They just distract the reviewer, and your formatting changes will probably not survive the next pgindent run anyway. * try to follow the project's code-layout conventions; again, this makes it easier for the reviewer, and there's no long-term point in trying to do it differently than pgindent would. > ! If your patch changes any existing defaults, you will need > to > ! explain why this is *required* or the patch will likely be > rejected. > ! New feature patches should also be accompanied by doc patches, > and > ! pointers to any relevant sections of the SQL standard are > recommended > ! as well. See 1.16 for more information on > the > ! SQL standards The above should be two items not one --- as written it downplays the importance of providing documentation, which is something we must talk up not down. (Bruce's original wording of the FAQ item I think underplays it; we should absolutely not give the impression that documentation is optional.) I'm not sticky about the docs being properly-marked-up SGML, but I think you should at least have expended the effort to explain what you are doing in English separate from the code. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Patch Submission Guidelines
On Tuesday 14 February 2006 20:42, Robert Treat wrote:
> On Tuesday 14 February 2006 16:00, Martijn van Oosterhout wrote:
> > > I would like to suggest that we increase substantially the FAQ entries
> > > relating to patch submission. By we, I actually mean please could the
> > > committers sit down and agree some clarified written guidelines?
> >
> > As I remember, there is a disinclination to increase the size of the
> > FAQ very much. This suggests maintaining it as a seperate document. Or
> > alternatively attach it as an appendix to the main documentation.
>
> Huh? The current developers FAQ is at least 1/2 the size of the main FAQ.
> I think adding a submission on patch submission guidelines is a great idea.
> I'll have a patch based on Simon's post to -patches ready in the next 24
> hours unless someone is really going to object.
As stated, the following patch adds a list of patch submission guidelines
based on Simon Riggs suggestions to the developers FAQ.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Index: doc/src/FAQ/FAQ_DEV.html
===
RCS file: /projects/cvsroot/pgsql/doc/src/FAQ/FAQ_DEV.html,v
retrieving revision 1.107
diff -c -r1.107 FAQ_DEV.html
*** doc/src/FAQ/FAQ_DEV.html 24 Dec 2005 19:29:38 - 1.107
--- doc/src/FAQ/FAQ_DEV.html 16 Feb 2006 04:44:57 -
***
*** 156,180
1.5) I've developed a patch, what next?
! Generate the patch in contextual diff format. If you are
! unfamiliar with this, you might find the script
! src/tools/makediff/difforig useful. Unified diffs are
! only preferrable if the file changes are single-line changes and
! do not rely on the surrounding lines.
!
! Ensure that your patch is generated against the most recent
! version of the code. If it is a patch adding new functionality, the
! most recent version is CVS HEAD; if it is a bug fix, this will be
! the most recently version of the branch which suffers from the bug
! (for more on branches in PostgreSQL, see 1.15).
!
! Finally, submit the patch to [EMAIL PROTECTED] It
will be reviewed by other contributors to the project and will be
! either accepted or sent back for further work. Also, please try to
! include documentation changes as part of the patch. If you can't do
! that, let us know and we will manually update the documentation when
! the patch is applied.
1.6) Where can I learn more about the
code?
--- 156,226
1.5) I've developed a patch, what next?
! You will need to submit the patch to [EMAIL PROTECTED] It
will be reviewed by other contributors to the project and will be
! either accepted or sent back for further work. To help ensure your patch
! is reviewed and committed in a timely fasion, please make sure your
! submission conforms to the following guidelines before sending your email:
!
! Has patch been discussed previously? If it has, give a direct link
! to the message and/or bug# from the mail archives
! (http://archives.postgresql.org/";>http://archives.postgresql.org/).
! If it has not and the patch is of any complexity it is strongly
! recommended you post a message to the appropriate list or you risk
! getting your patch rejected. Refer back to 1.4 for
! email guidelines.
!
! Ensure that your patch is generated against the most recent version
! of the code. If you are developing new features, this should be
! CVS HEAD; if it is a bug fix, this will be the most recent version of
! the branch which suffers from the bug. For more on branches in
! PostgreSQL, see 1.15.
!
! The patch should be generated in contextual diff format and should
! be applicable from the root directory. If you are unfamiliar with
! this, you might find the script src/tools/makediff/difforig
! useful. Unified diffs are only preferrable if the file changes are
! single-line changes and do not rely on the surrounding lines.
!
! PostgreSQL is licensed under a BSD license, so any submissions must
! conform to the BSD license to be included. If you use code that is
! available under some other license that is BSD compatible (eg. public
! domain) please note that code in your email submission
!
! Confirm that your changes can pass make check and list the
! platforms you have tested this on. If your changes are port specific,
! list the ports that it applies to.
!
! Provide an implementation overview, preferably in code comments.
!
! If it is a performance patch, provide confirming test results to
! show the benefits of your patch. It is OK to post patches without
! this information, though the patch will not be applied until *somebody*
! has tested the patches and found a valuable performance effect directly
! attributable to the patch. Given that writing performance tests is not
!
[PATCHES] constant too large in port/gettimeofday
This patch fixes this warning. gettimeofday.c:35: warning: integer constant is too large for "long" type Kris Jurka Index: src/port/gettimeofday.c === RCS file: /projects/cvsroot/pgsql/src/port/gettimeofday.c,v retrieving revision 1.7 diff -c -r1.7 gettimeofday.c *** src/port/gettimeofday.c 16 Dec 2005 21:55:27 - 1.7 --- src/port/gettimeofday.c 16 Feb 2006 02:17:23 - *** *** 32,38 /* FILETIME of Jan 1 1970 00:00:00. */ ! static const unsigned __int64 epoch = 1164447360L; /* * timezone information is stored outside the kernel so tzp isn't used anymore. --- 32,38 /* FILETIME of Jan 1 1970 00:00:00. */ ! static const unsigned __int64 epoch = 1164447360LL; /* * timezone information is stored outside the kernel so tzp isn't used anymore. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] patch fixing the old RETURN NEXT bug
"Sergey E. Koposov" <[EMAIL PROTECTED]> writes: > Are there any problems with that patch to not be applied ? Hasn't been reviewed yet ... see nearby discussions about shortage of patch reviewers ... > (Sorry if I'm hurrying) At the moment it's not unusual for nontrivial patches to sit around for a month or two, unless they happen to attract special interest of one of the committers. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] patch fixing the old RETURN NEXT bug
On Sun, 12 Feb 2006, Tom Lane wrote: > "Sergey E. Koposov" <[EMAIL PROTECTED]> writes: > > The problem with that is in fact in pl_exec.c in function > > compatible_tupdesc(), which do not check for the deleted attributes. > > Is that really the only problem? Tom, Are there any problems with that patch to not be applied ? (Sorry if I'm hurrying) Regards, Sergey * Sergey E. Koposov Max Planck Institute for Astronomy Web: http://lnfm1.sai.msu.ru/~math E-mail: [EMAIL PROTECTED] ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] BUG #2246: Only call pg_fe_getauthname if none given
Stephen Frost <[EMAIL PROTECTED]> writes: > Perhaps I'm missing something obvious (and if so, I'm sorry) but > couldn't we just build up the character array in PQsetdbLogin to be > passed in to connectOptions1? That's a possibility too, though by the time you've finished building that string (with appropriate quoting) and then tearing it apart again in conninfo_parse, it's likely that you've eaten the cycles you hoped to save ... especially since that processing would happen for all uses of PQsetdbLogin, whether or not they could avoid calling pg_fe_getauthname. I'm starting to feel that it's not worth messing with, which is obviously the same conclusion we came to last time round ... regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] BUG #2246: Only call pg_fe_getauthname if none given
* Tom Lane ([EMAIL PROTECTED]) wrote: > This is probably not a good idea --- changing the API behavior in > pursuit of saving a few cycles is just going to get people mad at us. Fair enough. > I think we'd have to refactor the code so that PQsetdbLogin gets a > PQconninfoOption array, overrides values *in that array*, then calls the > fallback-substitution code etc. Not sure if it's worth the trouble. > The extra complexity of searching the array for values to override could > eat up the cycles we're hoping to save, too :-( Perhaps I'm missing something obvious (and if so, I'm sorry) but couldn't we just build up the character array in PQsetdbLogin to be passed in to connectOptions1? If we do that we could probably also merge the two connectOptions... That would simplify things a great deal I think and would also avoid the extra processing to pick up the 'defaults'... Stephen signature.asc Description: Digital signature
Re: [PATCHES] BUG #2246: Only call pg_fe_getauthname if none given
Stephen Frost <[EMAIL PROTECTED]> writes: > * Tom Lane ([EMAIL PROTECTED]) wrote: >> Right offhand I like the idea of pushing it into connectOptions2 --- can >> you experiment with that? Seems like there is no reason to call >> Kerberos if the user supplies the name to connect as. > Patch attached. After looking through the code around this I discovered > that conninfo_parse is also called by PQconndefaults. This patch > changes the 'default' returned for user to NULL (instead of whatever > pg_fe_getauthname returns). This is probably not a good idea --- changing the API behavior in pursuit of saving a few cycles is just going to get people mad at us. After looking more closely, I see that there is inefficiency only in the PQsetdbLogin case --- in the PQconnectdb case, we only bother to compute "fallback" values for parameters where they're actually needed. I think that at the time this code was last restructured, the assumption was that PQsetdbLogin would die soon and there wasn't any need to worry about whether it wastes a few cycles. If you don't subscribe to that argument, probably the best answer is to keep the fallback-computation loop (fe-connect.c lines 2681-2735 in CVS tip) more or less as-is, but split it out from conninfo_parse, and somehow arrange for PQsetdbLogin to be able to insert the values that it wants before the fallbacks are computed. The trick here is that PQsetdbLogin wants to work on a PGconn not a PQconninfoOption array, and that isn't going to work nicely. I think we'd have to refactor the code so that PQsetdbLogin gets a PQconninfoOption array, overrides values *in that array*, then calls the fallback-substitution code etc. Not sure if it's worth the trouble. The extra complexity of searching the array for values to override could eat up the cycles we're hoping to save, too :-( regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
[PATCHES] BUG #2246: Only call pg_fe_getauthname if none given
* Tom Lane ([EMAIL PROTECTED]) wrote:
> Right offhand I like the idea of pushing it into connectOptions2 --- can
> you experiment with that? Seems like there is no reason to call
> Kerberos if the user supplies the name to connect as.
Patch attached. After looking through the code around this I discovered
that conninfo_parse is also called by PQconndefaults. This patch
changes the 'default' returned for user to NULL (instead of whatever
pg_fe_getauthname returns). This is probably better than not calling
Kerberos in pg_fe_getauthname and potentially returning the wrong thing
(if the username doesn't match the princ, a common situation), but it
might break existing applications. Are those applications wrong to be
asking libpq to provide what it thinks the username is when asking for
the defaults? I'd say probably yes, but then we don't provide another
way for them to get it either.
Patch tested w/ 'trust', 'ident', 'md5' and 'krb5' methods from psql.
Enjoy,
Stephen
Index: src/interfaces/libpq/fe-connect.c
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.326
diff -c -r1.326 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 13 Feb 2006 22:33:57 - 1.326
--- src/interfaces/libpq/fe-connect.c 15 Feb 2006 18:21:06 -
***
*** 96,105
* fallback is available. If after all no value can be determined
* for an option, an error is returned.
*
- * The value for the username is treated specially in conninfo_parse.
- * If the Compiled-in resource is specified as a NULL value, the
- * user is determined by pg_fe_getauthname().
- *
* The Label and Disp-Char entries are provided for applications that
* want to use PQconndefaults() to create a generic database connection
* dialog. Disp-Char is defined as follows:
--- 96,101
***
*** 423,434
--- 419,448
*
* Compute derived connection options after absorbing all user-supplied info.
*
+ * The value for the username is treated specially in connectOptions2.
+ * If the Compiled-in resource is specified as a NULL value and the user
+ * doesn't provide a username to use then the user is determined by
+ * calling pg_fe_getauthname().
+ *
* Returns true if OK, false if trouble (in which case errorMessage is set
* and so is conn->status).
*/
static bool
connectOptions2(PGconn *conn)
{
+ charerrortmp[PQERRORMSG_LENGTH];
+
+ /*
+* Find username to use if none given. This may call
+* additional helpers (ie: krb5) if those auth methods
+* are compiled in.
+*/
+ if (conn->pguser == NULL || conn->pguser[0] == '\0')
+ {
+ conn->pguser = pg_fe_getauthname(errortmp);
+ /* note any error message is thrown away */
+ }
+
/*
* If database name was not given, default it to equal user name
*/
***
*** 2505,2511
char *cp2;
PQconninfoOption *options;
PQconninfoOption *option;
- charerrortmp[PQERRORMSG_LENGTH];
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
--- 2519,2524
***
*** 2722,2737
}
continue;
}
-
- /*
-* Special handling for user
-*/
- if (strcmp(option->keyword, "user") == 0)
- {
- option->val = pg_fe_getauthname(errortmp);
- /* note any error message is thrown away */
- continue;
- }
}
return options;
--- 2735,2740
---(end of broadcast)---
TIP 4: Have you searched our list archives?
http://archives.postgresql.org
Re: [PATCHES] add additional options to CREATE TABLE ... AS
On Tue, 2006-02-14 at 15:38 -0500, Tom Lane wrote: > Neil Conway <[EMAIL PROTECTED]> writes: > > The implementation is pretty ugly -- it clutters ExecuteStmt and Query > > with fields that really do not belong there. Per previous discussion, I > > think it would be better to refactor the CREATE TABLE AS implementation > > to be essentially a CREATE TABLE followed by a INSERT ... SELECT. > > I kinda wonder why bother at all. I don't see any good reason why > people shouldn't issue two statements. The good reason is that CREATE plus INSERT SELECT is extremely bug prone and very annoying to have to code SQL that way. CTAS is sent from on high, IMHO, even without the performance optimisation. Best Regards, Simon Riggs ---(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
