[PATCHES] libpq events patch
This is an updated version pf the libpqevents patch. See http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php for details. The only change I didn't make yet is the event 'name'. I have put it in and taken it out twice now, so a firm 'put it in there' would be appreciated. Go here for libpqtypes using events. pgfoundry is still using the older object hooks version. http://libpqtypes.esilo.com/libpqtypes-events.tar.gz -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/interfaces/libpq/Makefile === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/Makefile,v retrieving revision 1.166 diff -C6 -r1.166 Makefile *** src/interfaces/libpq/Makefile 16 Apr 2008 14:19:56 - 1.166 --- src/interfaces/libpq/Makefile 3 Sep 2008 16:06:49 - *** *** 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif --- 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o libpq-events.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif *** *** 103,114 --- 103,115 $(top_builddir)/src/port/pg_config_paths.h: $(MAKE) -C $(top_builddir)/src/port pg_config_paths.h install: all installdirs install-lib $(INSTALL_DATA) $(srcdir)/libpq-fe.h '$(DESTDIR)$(includedir)' + $(INSTALL_DATA) $(srcdir)/libpq-events.h '$(DESTDIR)$(includedir)' $(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample' installdirs: installdirs-lib $(mkinstalldirs) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' Index: src/interfaces/libpq/exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** src/interfaces/libpq/exports.txt19 Mar 2008 00:39:33 - 1.19 --- src/interfaces/libpq/exports.txt3 Sep 2008 16:06:49 - *** *** 138,143 --- 138,154 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQcopyResult 142 + PQsetResultAttrs 143 + PQsetvalue144 + PQresultAlloc 145 + PQregisterEventProc 146 + PQinstanceData147 + PQsetInstanceData 148 + PQresultInstanceData 149 + PQresultSetInstanceData 150 + PQpassThroughData 151 + PQresultPassThroughData 152 Index: src/interfaces/libpq/fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.359 diff -C6 -r1.359 fe-connect.c *** src/interfaces/libpq/fe-connect.c 29 May 2008 22:02:44 - 1.359 --- src/interfaces/libpq/fe-connect.c 3 Sep 2008 16:06:49 - *** *** 1971,1982 --- 1971,2000 * release data that is to be held for the life of the PGconn structure. * If a value ought to be cleared/freed during PQreset(), do it there not here. */ static void freePGconn(PGconn *conn) { + int i; + PGEventConnDestroy evt; + + /* Let the event procs cleanup their state data */ + for(i=0; i conn-nEvents; i++) + { + evt.conn = conn; + (void)conn-events[i].proc(PGEVT_CONNDESTROY, evt); + } + + /* free the PGEvent array */ + if(conn-events) + { + free(conn-events); + conn-events = NULL; + conn-nEvents = conn-eventArrSize = 0; + } + if (conn-pghost) free(conn-pghost); if (conn-pghostaddr) free(conn-pghostaddr
Re: [PATCHES] libpq events patch
Andrew Chernow wrote: This is an updated version pf the libpqevents patch. See http://archives.postgresql.org/pgsql-hackers/2008-09/msg00153.php for details. The only change I didn't make yet is the event 'name'. I have put it in and taken it out twice now, so a firm 'put it in there' would be appreciated. Go here for libpqtypes using events. pgfoundry is still using the older object hooks version. http://libpqtypes.esilo.com/libpqtypes-events.tar.gz Patch update again. This one includes an optional event name for debugging purposes. It also includes the changes made by Alvaro that I missed. PQregisterEventProc now takes a name argumet. If the name is NULL, the error message will identify the event procedure by its address ... addr:%p. If its not NULL, error messages will indicate the provided name. I updated the styling in libpq-events.c and a couple places in fe-exec.c that Alvaro missed. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/interfaces/libpq/Makefile === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/Makefile,v retrieving revision 1.166 diff -C6 -r1.166 Makefile *** src/interfaces/libpq/Makefile 16 Apr 2008 14:19:56 - 1.166 --- src/interfaces/libpq/Makefile 3 Sep 2008 21:55:06 - *** *** 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif --- 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o libpq-events.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif *** *** 103,123 $(top_builddir)/src/port/pg_config_paths.h: $(MAKE) -C $(top_builddir)/src/port pg_config_paths.h install: all installdirs install-lib $(INSTALL_DATA) $(srcdir)/libpq-fe.h '$(DESTDIR)$(includedir)' $(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample' installdirs: installdirs-lib $(mkinstalldirs) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' uninstall: uninstall-lib ! rm -f '$(DESTDIR)$(includedir)/libpq-fe.h' '$(DESTDIR)$(includedir_internal)/libpq-int.h' '$(DESTDIR)$(includedir_internal)/pqexpbuffer.h' '$(DESTDIR)$(datadir)/pg_service.conf.sample' clean distclean: clean-lib rm -f $(OBJS) pg_config_paths.h crypt.c getaddrinfo.c inet_aton.c noblock.c open.c pgstrcasecmp.c snprintf.c strerror.c strlcpy.c thread.c md5.c ip.c encnames.c wchar.c win32error.c pgsleep.c pthread.h libpq.rc # Might be left over from a Win32 client-only build rm -f pg_config_paths.h --- 103,124 $(top_builddir)/src/port/pg_config_paths.h: $(MAKE) -C $(top_builddir)/src/port pg_config_paths.h install: all installdirs install-lib $(INSTALL_DATA) $(srcdir)/libpq-fe.h '$(DESTDIR)$(includedir)' + $(INSTALL_DATA) $(srcdir)/libpq-events.h '$(DESTDIR)$(includedir)' $(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample' installdirs: installdirs-lib $(mkinstalldirs) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' uninstall: uninstall-lib ! rm -f '$(DESTDIR)$(includedir)/libpq-fe.h' '$(DESTDIR)$(includedir)/libpq-events.h' '$(DESTDIR)$(includedir_internal)/libpq-int.h' '$(DESTDIR)$(includedir_internal)/pqexpbuffer.h' '$(DESTDIR)$(datadir)/pg_service.conf.sample' clean distclean: clean-lib rm -f $(OBJS) pg_config_paths.h crypt.c getaddrinfo.c inet_aton.c noblock.c open.c pgstrcasecmp.c snprintf.c strerror.c strlcpy.c thread.c md5.c ip.c encnames.c wchar.c win32error.c pgsleep.c pthread.h libpq.rc # Might be left over from a Win32 client
Re: [HACKERS][PATCHES] odd output in restore mode
Heikki Linnakangas wrote: Andrew Dunstan wrote: - or maybe provide a .bat file or perl script that would work as na archive_command on Windows. We're not talking about archive_command. We're talking about the thing that copies files to the directory that pg_standby polls. Er, that's what the archive_command is. Look at the pg_standby docs and you'll see that that's where we're currently recommending use of windows copy. Perhaps you're confusing this with the restore_command? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [HACKERS][PATCHES] odd output in restore mode
Greg Smith wrote: On Wed, 23 Jul 2008, Kevin Grittner wrote: In our scripts we handle this by copying to a temp directory on the same mount point as the archive directory and doing a mv to the archive location when the copy is successfully completed. I think that this even works on Windows. Could that just be documented as a strong recommendation for the archive script? This is exactly what I always do. I think the way cp is shown in the examples promotes what's really a bad practice for lots of reasons, this particular problem being just one of them. I've been working on an improved archive_command shell script that I expect to submit for comments and potential inclusion in the documentation as a better base for other people to build on. This is one of the options for how it can operate. It would be painful but not impossible to convert a subset of that script to run under Windows as well, at least enough to cover this particular issue. A Perl script using the (standard) File::Copy module along with the builtin function rename() should be moderately portable. It would to be nice not to have to maintain two scripts. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] pg_dump additional options for performance
Joshua D. Drake wrote: Agreed but that is a problem I understand with a solution I don't. I am all eyes on a way to fix that. One thought I had and please, be gentle in response was some sort of async transaction capability. I know that libpq has the ability to send async queries. Is it possible to do this: send async(copy table to foo) send async(copy table to bar) send async(copy table to baz) Where all three copies are happening in the background? IIRC, libpq doesn't let you have more than one async query active at one time. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [HACKERS][PATCHES] odd output in restore mode
Kevin Grittner wrote: Heikki Linnakangas [EMAIL PROTECTED] wrote: We really need a more reliable way of detecting that a file has been fully copied. In our scripts we handle this by copying to a temp directory on the same mount point as the archive directory and doing a mv to the archive location when the copy is successfully completed. I think that this even works on Windows. Could that just be documented as a strong recommendation for the archive script? Needs testing at least. If it does in fact work then we can just adjust the docs and be done - or maybe provide a .bat file or perl script that would work as na archive_command on Windows. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] pg_dump additional options for performance
Tom Lane wrote: Simon Riggs [EMAIL PROTECTED] writes: I also suggested having three options --want-pre-schema --want-data --want-post-schema so we could ask for any or all parts in the one dump. --data-only and --schema-only are negative options so don't allow this. (I don't like those names either, just thinking about capabilities) Maybe invert the logic? --omit-pre-data --omit-data --omit-post-data Not wedded to these either, just tossing out an idea... Please, no. Negative logic seems likely to cause endless confusion. I'd even be happier with --schema-part-1 and --schema-part-2 if we can't find some more expressive way of designating them. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Solaris ident authentication using unix domain sockets
Josh Berkus wrote: Tom, Indeed. If the Solaris folk feel that getupeercred() is insecure, they had better explain why their kernel is that broken. This is entirely unrelated to the known shortcomings of the ident IP protocol. The Solaris security kernel folks do, actually. However, there's no question that TRUST is inherently insecure, and that's what people are going to use if they can't get IDENT to work. I think I'd pose a slightly different question from Tom. Do the Solaris devs think that their getupeercred() is more insecure than the more or less equivalent calls that we are doing on Linux and *BSD for example? I suspect they probably don't ;-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Solaris ident authentication using unix domain sockets
Robert Treat wrote: On Thursday 03 July 2008 14:01:22 Tom Lane wrote: Garick Hamlin [EMAIL PROTECTED] writes: I have a patch that I have been using to support postgresql's notion of ident authentication when using unix domain sockets on Solaris. This patch basically just adds support for using getupeercred() on Solaris so unix sockets and ident auth works just like it does on Linux and elsewhere. Cool. Hmm... I've always been told that Solaris didn't support this because the Solaris developers feel that IDENT is inherently insecure. If that is more than just a philosphical opinion, I wonder if there should be additional hurdles in place to enable this on that platform. Note that isn't an objection from me, though I'm curious if any of the Sun guys want to chime in on this. We don't actually use the Ident protocol for Unix sockets on any platform. AIUI, this patch just implements what we do on platforms like Linux or *BSD. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] variadic function support
Pavel Stehule wrote: I afraid so Java syntax isn't good inspiration http://www.java-tips.org/java-se-tips/java.lang/using-the-varargs-language-feature.html http://www.clanproductions.com/java5.html they use symbol ... like specific synonym to []. public Method getMethod(String name, Class... parameterTypes) Well, ... is really more the equivalent of your variadic keyword, I think. public Method getMethod(String name, Class[] ... parameterTypes) would mean each variadic argument would be an array of Class. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] variadic function support
Pavel Stehule wrote: Hello this patch enhance current syntax of CREATE FUNCTION statement. It allows creating functions with variable number of arguments. This version is different than last my patches. It doesn't need patching PL. Basic idea is transformation of real arguments (related to declared variadic argument) to array. All changes are mostly in parser. Demo: CREATE FUNCTION public.least(double precision[]) RETURNS double precision AS $$ SELECT min($1[i]) FROM generate_subscripts($1,1) g(i) $$ LANGUAGE SQL VARIADIC; SELECT public.least(3,2,1); least --- 1 (1 row) SELECT public.least(3,2,1,0,-1); least --- -1 CREATE FUNCTION concat(varchar, anyarray) RETURNS varchar AS $$ SELECT array_to_string($2, $1); $$ LANGUAGE SQL VARIADIC; SELECT concat('-',2008,10,12); concat 2008-10-12 (1 row) And what about a function that takes 2 arrays as arguments? This proposal strikes me as half-baked. Either we need proper and full support for variadic functions, or we don't, but I don't think we need syntactic sugar like the above (or maybe in this case it's really syntactic saccharine). cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] variadic function support
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Tom Lane wrote: What would you consider proper and full support? I don't know. But this doesn't feel like it. That's a fairly weak argument for rejecting a patch that provides a feature many people have asked for. OK. Let me be a bit more specific. I think (forcing myself to be a bit more analytic than I have been up to now) my main objection is that the variadic part of the parameters should be marked explicitly in the formal parameter list. I don't mind having it limited to a single typed array - as you say we probably don't want someone implementing sprintf. But if I have foo( a text, b int[]) it looks odd if both these calls are legal: foo('a',1,2,3,) foo('a',ARRAY[1,2,3]) which I understand would be the case with the current patch. I'm also still curious to know how the following would be handled: foo(a text[], b text[]) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] variadic function support
Tom Lane wrote: Your point about the syntax is good though. It would be better if the syntax were like create function foo (a text, variadic b int[]) or maybe even better create function foo (a text, variadic b int) since (a) this makes it much more obvious to the reader what the function might match, and (b) it leaves the door open for marking multiple parameters as variadic, if we can figure out what that means. Yes. I understand from the family Java expert that (surface syntax issues aside) the second is similar to the way Java does this, in fact, so there's some precedent. That would mean that your first would actually mean each variadic arg has to be an array of ints, which we might well want to provide for. So with that modification I'll be lots happier with the feature. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] doc patch - archive/restore_command on windows
ITAGAKI Takahiro wrote: I found the examples of documentation about archive_command and restore_command for Windows are incorrect or improper. - copy doesn't accept / (slash) as a path separator. We should use \\ (double backslash) for the purpose. - Windows path is typically starts with a drive letter (C:\\). - We'd better to quote a whole path, not only the last filename with double-quotes. It can work, but is not a windows manner. As previously discussed, we should probably stop recommending use of the Windows copy command altogether, and recommend use of GnuWin32 cp instead, for archive_command. The latter does behave sanely w.r.t. forward slashes. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
Tom Lane wrote: multi-object DROP IF NOT EXISTS would fail to perform as expected. Surely this would be a noop :-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Feature: give pg_dump a WHERE clause expression
Tom Lane wrote: Davy Durham [EMAIL PROTECTED] writes: So, if this patch is not acceptable as-is, what would you feel about this: I could enhance the -t/--table=NAME option to accept more than a simple NAME. Rather it could accept something in the form: --table=table_name:where-clause expression Well, that would at least address the complaint that it doesn't scale to multiple tables, but the whole thing still seems like a frammish that will never see enough use to justify maintaining it. (BTW, what will you do with a table whose name contains a colon?) ISTM this would be better off waiting until we turn large parts of pg_dump into a library, as has been often discussed, at which point it should be relatively simple to write a custom client to do what the OP wants. I agree that it does not at all belong in pg_dump. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Feature: give pg_dump a WHERE clause expression
daveg wrote: ISTM this would be better off waiting until we turn large parts of pg_dump into a library, as has been often discussed, at which point it should be relatively simple to write a custom client to do what the OP wants. I agree that it does not at all belong in pg_dump. I can't imagine many of my clients ever writing another C program or even being willing to pay me to do so. While modularizing pg_dump is a fine idea, I don't think it addresses the same set of use cases and users as this proposal. It's not clear to me that your use case is very compelling. Does your foreign database not support import via CSV or XML? Postgres can now produce both of these for any arbitrary query. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks (libpq events)
Will make all of those changes. We appreciate the help. 1. remove global registration :( 2. New Name: PGCallback 3. use instanceData and passThrough names (passThrough with upper 'T') 3. separate getters for conn/result instanceData and passthrough 4. add a setter for result instance data - There should also be a PQsetInstanceData(PGconn*, ...) - I see no need for a passThrough setter 5. move callback stuff to its own header, maybe pgcallback.h? No issue with any of them. Although, small issue below: Maybe instead of having the ResultCreate callback scribble on the event data structure, provide an additional API routine to store the pointer: PQresultSetInstanceData(PGresult *res, PGeventProc proc, void *instanceData); Adding PQresultSetInstanceData doesn't removes the need for a resultcreate callback event. This is an event the callbacks are informed about (instanceData or not). It does remove the need for an instance data member in all event info structures, just use the getter/setter when desired. If the passThrough is needed, one can use the public getters. hooks registered. Also, meseems you need such a callback anyway: what if the hook library desires to realloc its instance data larger? With your suggestions, this would work: res = PQexec(conn, blah); data = PQresultInstanceData(res, cbfunc); data = realloc(data, 1024); PQresultSetInstanceData(res, cbfunc, data); The API user should have a valid instanceData whenever libpq returns a result, assuming they registered a callback that allocates instance data during a resultcreate event. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks (libpq events)
Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: 4. add a setter for result instance data - There should also be a PQsetInstanceData(PGconn*, ...) - I see no need for a passThrough setter Check, though I assume we're not expecting PQsetInstanceData to propagate to previously created PGresults? No, not at all. Already created results are on their own. If you want to modify the instanceData, you can use PQresultSetInstanceData. 5. move callback stuff to its own header, maybe pgcallback.h? Should be libpq-something. I was considering libpq-hooks.h or libpq-events.h, but libpq-callback.h would be OK too. I like events. It sounds like you wanted PGCallback, although I am starting to think PGEventProc is better than PGcallback, only because it is more consistent with the term events. BTW, my suggestion to call this libpq events was not directly referring to the callback/proc. It was a term for describing the whole #! I think the idea is to notify interested parties about libpq events, the callback is just an implementaion for doing that. Adding PQresultSetInstanceData doesn't removes the need for a resultcreate callback event. No, of course not. What I was imagining was that the resultcreate callback would call PQresultSetInstanceData. Sorry, my mistake. You were actually very clear, my reading skills are in question. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks (libpq events)
Attached is the latest patch. It has addressed the requested changes found here: http://archives.postgresql.org/pgsql-patches/2008-05/msg00389.php Its a tarball because there are two new files, libpq-events.c and libpq-events.h. The patch is in the tarball as well as attached to the email. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ libpq-events.tgz Description: application/compressed Index: src/interfaces/libpq/Makefile === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/Makefile,v retrieving revision 1.166 diff -C6 -r1.166 Makefile *** src/interfaces/libpq/Makefile 16 Apr 2008 14:19:56 - 1.166 --- src/interfaces/libpq/Makefile 20 May 2008 04:18:07 - *** *** 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif --- 29,41 # the object files from libpgport, this would not be true on all # platforms. LIBS := $(LIBS:-lpgport=) OBJS= fe-auth.o fe-connect.o fe-exec.o fe-misc.o fe-print.o fe-lobj.o \ fe-protocol2.o fe-protocol3.o pqexpbuffer.o pqsignal.o fe-secure.o \ ! md5.o ip.o wchar.o encnames.o noblock.o pgstrcasecmp.o thread.o libpq-events.o \ $(filter crypt.o getaddrinfo.o inet_aton.o open.o snprintf.o strerror.o strlcpy.o win32error.o, $(LIBOBJS)) ifeq ($(PORTNAME), cygwin) override shlib = cyg$(NAME)$(DLSUFFIX) endif *** *** 103,114 --- 103,115 $(top_builddir)/src/port/pg_config_paths.h: $(MAKE) -C $(top_builddir)/src/port pg_config_paths.h install: all installdirs install-lib $(INSTALL_DATA) $(srcdir)/libpq-fe.h '$(DESTDIR)$(includedir)' + $(INSTALL_DATA) $(srcdir)/libpq-events.h '$(DESTDIR)$(includedir)' $(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample' installdirs: installdirs-lib $(mkinstalldirs) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' Index: src/interfaces/libpq/exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** src/interfaces/libpq/exports.txt 19 Mar 2008 00:39:33 - 1.19 --- src/interfaces/libpq/exports.txt 20 May 2008 04:18:07 - *** *** 138,143 --- 138,153 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQcopyResult 142 + PQsetvalue143 + PQresultAlloc 144 + PQregisterEventProc 145 + PQinstanceData146 + PQsetInstanceData 147 + PQresultInstanceData 148 + PQresultSetInstanceData 149 + PQpassThroughData 150 + PQresultPassThroughData 151 \ No newline at end of file Index: src/interfaces/libpq/fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.358 diff -C6 -r1.358 fe-connect.c *** src/interfaces/libpq/fe-connect.c 16 May 2008 18:30:53 - 1.358 --- src/interfaces/libpq/fe-connect.c 20 May 2008 04:18:08 - *** *** 1970,1981 --- 1970,1999 * release data that is to be held for the life of the PGconn structure. * If a value ought to be cleared/freed during PQreset(), do it there not here. */ static void freePGconn(PGconn *conn) { + int i; + PGEventConnDestroy evt; + + /* Let the event procs cleanup their state data */ + for(i=0; i conn-nEvents; i++) + { + evt.conn = conn; + (void)conn-events[i].proc(PGEVT_CONNDESTROY, evt); + } + + /* free the PGEvent array */ + if(conn-events) + { + free(conn-events); + conn-events = NULL; + conn-nEvents = conn-eventArrSize = 0; + } + if (conn-pghost) free(conn-pghost); if (conn-pghostaddr) free(conn-pghostaddr); if (conn-pgport) free(conn-pgport); *** *** 2151,2164 PQreset(PGconn *conn) { if (conn) { closePGconn(conn); ! if (connectDBStart(conn)) ! (void) connectDBComplete(conn); } } /* * PQresetStart: --- 2169,2199 PQreset(PGconn *conn) { if (conn) { closePGconn(conn); ! if (connectDBStart
Re: [PATCHES] [HACKERS] use of pager on Windows psql
Bruce Momjian wrote: Andrew Dunstan wrote: Not sure why ware are not. Should we enabled that code on Win32 and see how it works? Can you test it? Was it some MinGW limitation? I do see isatty() being used on lots of platforms. This is kind of odd. Ah, I bet it came from libpq's PQprint(), which I think we had working on Win32 long before we had psql working and perhaps I copied it from there. I don't see the Win32 checks around isatty() anywhere else. In fact, it looks to me like it would be much more sensible to #include settings.h and then simply test pset.notty for all platforms. Yes, we could do that but does the isatty() value ever change while psql is running? When you do '\g filename' does stdout then have isatty as false? Good point. I think the best thing would just be to remove the #ifndef WIN32 / #endif lines OK, patch applied to remove the Win32 test in both places. This broke the buildfarm and finally explains the following kluge which has been puzzling me for four years: /* * for some reason MinGW (and MSVC) outputs an extra newline, so this * suppresses it */ #ifndef WIN32 fputc('\n', fout); #endif I have removed the kluge (and yes, I tested it). cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] use of pager on Windows psql
Bruce Momjian wrote: This broke the buildfarm and finally explains the following kluge which has been puzzling me for four years: /* * for some reason MinGW (and MSVC) outputs an extra newline, so this * suppresses it */ #ifndef WIN32 fputc('\n', fout); #endif I have removed the kluge (and yes, I tested it). Oh, that kluge. Why did the isatty() addition fix this? Was the pager being used on Win32 for the regression tests and somehow eating a line or something? It apparently produced an extra line which we had compensated for with the kluge (without really understanding why we had to). Anyway, all is good now, as the buildfarm shows. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks (libpq events)
Merlin Moncure wrote: On Sat, May 17, 2008 at 8:28 AM, Andrew Chernow [EMAIL PROTECTED] wrote: Here is an updated patch for what was called object hooks. This is now called libpq events. If someone has a better name or hates ours, let us know. Let's decide where to go with this. We have no objections to pushing this back to the next fest (tom's comments on the wiki were noted). If that's the case though, we would like to wrap up a consenus on the general implementation in the meantime. Just give us a heads up and I'll update the wiki. merlin Yeah, it would be nice to avoid another push back into July by getting some feedback now for June; it would be great to squeeze it into May. I'm not talking about a review, but maybe a couple of I hate this or This works well or Give up coding :) The implementation of the events (as well as when they were object hooks) is actually very simple, so a quick glance can probably raise most concerns. There is very little going on. In the end, something like this can be done several different ways (choice here is a matter of taste style). It would be nice to iron out the API and focus on other aspects; namely is this general concept appealing enough. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks (libpq events)
Here is an updated patch for what was called object hooks. This is now called libpq events. If someone has a better name or hates ours, let us know. I am continuing to use the object hooks thread to avoid confusing anyone. Terminology: I got rid of calling it object events because it is possible to add other non-object-related events in the future; maybe a query event. This system can be used for any type of event, I think it is pretty generic. event proc - the callback procedure/function implemented outside of libpq ... PGEventProc. The address of the event proc is used as a lookup key for getting a conn/result's event data. event data - the state data managed by the event proc but tracked by libpq. This allows the event proc implementor to basically add a dynamic struct member to a conn or result. This is an instance based value, unlike the arg pointer. arg pointer - this is the passthrough/user pointer. I called it 'arg' as other libpq callbacks used this term for this type of value. This value can be retrieved via PQeventData, PQresultEventData ... its an optional double pointer argument. event state - an internal structure, PGEventState, which contains the event data, event proc and the 'arg' pointer. Internally, PGconn and PGresult contain arrays of event states. event id - PGEventId enum values, PGEVT_xxx. This tells the event proc which event has occurred. event info - These are the structures for event ids, like PGEVT_RESULTDESTROY has a corresponding PGEventResultDestroy structure. The PGEventProc function's 2nd argument is void *info. The first argument is an event id which tells the proc implementor how to cast the void *. This replaced the initial va_arg idea. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** exports.txt 19 Mar 2008 00:39:33 - 1.19 --- exports.txt 17 May 2008 12:06:10 - *** *** 138,143 --- 138,149 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQcopyResult 142 + PQsetvalue143 + PQresultAlloc 144 + PQregisterEventProc 145 + PQeventData 146 + PQresultEventData 147 Index: fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.358 diff -C6 -r1.358 fe-connect.c *** fe-connect.c16 May 2008 18:30:53 - 1.358 --- fe-connect.c17 May 2008 12:06:11 - *** *** 998,1009 --- 998,1014 * We really shouldn't have been polled in these two cases, but we * can handle it. */ case CONNECTION_BAD: return PGRES_POLLING_FAILED; case CONNECTION_OK: + if(!pqRegisterGlobalEvents(conn)) + { + conn-status = CONNECTION_BAD; + return PGRES_POLLING_FAILED; + } return PGRES_POLLING_OK; /* These are reading states */ case CONNECTION_AWAITING_RESPONSE: case CONNECTION_AUTH_OK: { *** *** 1816,1827 --- 1821,1837 conn-next_eo = EnvironmentOptions; return PGRES_POLLING_WRITING; } /* Otherwise, we are open for business! */ conn-status = CONNECTION_OK; + if(!pqRegisterGlobalEvents(conn)) + { + conn-status = CONNECTION_BAD; + return PGRES_POLLING_FAILED; + } return PGRES_POLLING_OK; } case CONNECTION_SETENV: /* *** *** 1848,1859 --- 1858,1874 default: goto error_return; } /* We are open for business! */ conn-status = CONNECTION_OK; + if(!pqRegisterGlobalEvents(conn)) + { + conn-status = CONNECTION_BAD; + return PGRES_POLLING_FAILED
Re: [PATCHES] libpq thread-locking
! int pthread_mutex_init(pthread_mutex_t *mp, void *attr) { *mp = CreateMutex(0, 0, 0); + if (*mp == NULL) + return 1; + return 0; } Maybe it would be better to emulate what pthreads does. Instead of returing 1 to indicate an error, return an errno. In the above case, ENOMEM seems like a good fit. Also, maybe you should check the passed in mutex pointer. If its NULL, you could return EINVAL. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Merlin Moncure wrote: Also, even if varargs are safe they'd be notationally unpleasant in the extreme. varargs are just a PITA to work with --- you'd have to do all the decoding in the first-level hook routine, even for items you weren't going to use. With something like the above all you need is a switch() and some pointer casts. Switch, plus struct (basically a union) will do the trick nicely. Can it be a formal union, or is it better as a void*? The main issue was how what we called the 'hook data' was passed back and forth. We'll get a patch up. All of this is getting quite a long way from what was in the commitfest queue. Do we still want to try to get this in this cycle, or should it be marked returned to author for more work? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: All of this is getting quite a long way from what was in the commitfest queue. Do we still want to try to get this in this cycle, or should it be marked returned to author for more work? So far I think it still falls within the category of allowing the author to revise his work. I don't want to hold commitfest open waiting on revisions of this patch, but as long as there's still other stuff being worked through I don't see why they can't keep trying. Just for the record, I would really like to close this fest before PGCon starts. We still have a couple more days to get it done. Apart from this one only two have not been claimed: . Map Forks . TRUNCATE TABLE with IDENTITY cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: typedef void (*PGeventProc) (PGeventId eventId, const void *eventInfo, void *passthrough); int PQregisterEventProc(PGconn *conn, PGeventProc proc, void *passthrough); The above prototypes will work and we will add our 'event instance pointer' to the event info structures. Should have a patch shortly. libpqtypes doesn't need a passthrough/user-pointer. The object events/hooks allocate memory when the object is created part of a conn/result object instance, it is not supplied by the API user registering the event/hook callback. I think this is where some confusion has been occurring, there are two different pointers: user pointer and event instance pointer. BTW, PQeventData and PQresultEventData return the event instance pointer, not the passthrough. At least that is how we were using these functions, being how our previous patches do not include a passthrough/user-pointer feature because libpqtypes didn't need it. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Where exactly does the hook hand off the storage pointer to libpq? What are you going to do if the hook fails to create the storage (ie, out of memory during PGresult creation)? The current submitted patch has 3 of its function callbacks returning a void*, initHookData after the creation of a conn, resultCreate, and resultCopy. We have recently changed this design so all hooks, now called events, go through a single callback ... PGEventProc. The old function callbacks are just eventIds now. / // The libpq side (looping registered event procs) PGEventResultCreate info; info.stateData = NULL; /* our event data ptr */ info.conn = conn; info.result = result; if(!result-evtState[i].proc(PGEVT_RESULTCREATE, (void *)info, result-evtState[i].passThrough) { PQclear(result); // destroy result? ... not sure return error;// previously, we ignored it } // assign event data created by event proc. result-evtState[i].data = info.stateData; /// // example of what the event proc does int my_evtproc(PGEventId evtId, void *evtInfo, void *passThrough) { switch(eventId) { case PGEVT_RESULTCREATE: { void *data = makeresultdata() if(!data) return FALSE; ((PGEventResultCreate *)evtInfo)-stateData = data; break; } } return TRUE; } -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: ISTM the hook ought to be able to request that libpq return an out-of-memory failure for the query, just as would happen if the malloc failure had happened directly to libpq. I am working on this new patch and that is how I have been implementing it. If the eventProc function returns FALSE for creation events (not destruction events), the libpq call that triggered it will fail. For instance: for the creation of result objects PGEVT_RESULTCREATE, I am clearing the result and returning an error value. I think that is the expected behavior when one calls PQexec and an out-of-memory failure occurs. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Patch to change psql default banner v6
Joshua D. Drake wrote: O.k. I am not trying to start an argument here but... I already sent 6 revisions of this patch that received comments and had thorough review via Alvaro. I even took into account Tom's original comments from the previous thread. This much effort on something so simple makes it not worth the effort in the first place. Welcome to UI development. There is always *far* more argument of minor matters of appearance than over anything else, in my experience. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Merlin Moncure [EMAIL PROTECTED] writes: The problem is the functions PQhookData(conn, hookname) and PQresultHookData(result, hookName). We need these to work in functions that are not callbacks. If we eliminate hookname completely, there is no way for libpq to know which private state we are asking for. Well, depending on a hook name for this is broken-by-design anyway, because there is no way for two independently written libraries to be sure they don't choose conflicting hook names. So the need for a hook name has to go away. It might work to use the address of the hook callback function as a key for retrieving the associated void * pointer. You'd need to not register the same callback function more than once per object, but from what I gather here you don't need to. Or else have the library return a unique handle when registering hooks, rather than supplying a hook name. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Merlin Moncure [EMAIL PROTECTED] writes: The problem is the functions PQhookData(conn, hookname) and PQresultHookData(result, hookName). We need these to work in functions that are not callbacks. If we eliminate hookname completely, there is no way for libpq to know which private state we are asking for. Well, depending on a hook name for this is broken-by-design anyway, because there is no way for two independently written libraries to be sure they don't choose conflicting hook names. So the need for a hook name has to go away. It might work to use the address of the hook callback function as a key for retrieving the associated void * pointer. You'd need to not register the same callback function more than once per object, but from what I gather here you don't need to. regards, tom lane There can be cases to use the same callbacks, although unlikely. To completely avoid collisions, the below would work: Use the address of a static, maybe an 'int', as a hook hanlde. Provide the user with a macro that can make a hook handle. typedef void *PGhookHandle; // Declare an int and point tokname at it. The value doesn't // matter, its the pointer address we are interested in. #define PQ_MAKE_HOOK_HANDLE(tokname) \ static int hh__ ## tokname = 0; \ static const PGhookHandle tokname = hh__ ## tokname As an example, here is what libpqtypes would do: // libpqtypes hooks.c PQ_MAKE_HOOK_HANDLE(pqthookhandle); Now the handle replaces the hookName. The const char *hookName member of the PQobjectHooks structure is changed to const PGhookHanlde hookHandle. This allows for all the flexibility of a const char * w/o the collision issues. // these function prototypes change as well void *PQhookData(PGconn *, const PGhookHandle); void *PQresultHookData(PGresult *, const PGhookHandle); We will send in an updated patch. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Andrew Dunstan wrote: Tom Lane wrote: It might work to use the address of the hook callback function as a key for retrieving the associated void * pointer. You'd need to not register the same callback function more than once per object, but from what I gather here you don't need to. Or else have the library return a unique handle when registering hooks, rather than supplying a hook name. cheers andrew The problem with this is that hooks can be registered on a per-conn basis. Is there a way to ensure the libpq returned handle would be the unique across every registration of a given PGobjectHooks? ISTM that the hook handle needs to be constant and unique somehow. Tom's idea would work with the very small limitation of not being able to reuse hook callbacks. I throw out an idea of using the address of a static, which is constant and unique. app_func(PGresult *res) { PQresultHookData(res, ?handle?); } app_func is not aware of what PGconn generated the result so it has no idea what libpq returned handle to use. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: There can be cases to use the same callbacks, although unlikely. To completely avoid collisions, the below would work: Still looks like overdesign to me. If we use the hook function address we solve the problem with no extra notation and no extra storage. Note that if you want N fixed keys, you can just have N hook functions (all calling a shared workhorse routine, no doubt). So your proposal adds no functionality whatever if the usage involves a fixed number of static handles. Now it could possibly allow a variable-at-runtime number of handles, but I'd want to see a worked-out use case before designing for that much complexity. In particular, it seems to me that the problem would then shift to how do you know which handle to use for the lookup, thus you've just introduced another layer of complexity without buying anything. I think the typical use case is just that you need to distinguish your hook from anyone else's hooks, so the function address is plenty sufficient. It should also be noted that this whole problem *can* be solved without any PQhookData at all: as long as you have hooks to get control at creation and destruction of PGconns and PGresults, you can maintain your own index data structure. I'm willing to grant some amount of extra API-stuff to save users having to do that in simple cases, but I don't think we need to try to support infinitely complex cases. regards, tom lane Okay. No problem over here. Which callback do we use as the key? Currently, none are required (only the name was required). We have to choose one callback that must be provided. Maybe initHookData() must be provided? If the end-user doesn't need it they can just return NULL. This is what is passed to PQaddObjectHooks, along with a conn: typedef struct { //char *name; REMOVED void *data; /* Invoked when PQsetObjectHook is called. The pointer returned * by the hook implementation is stored in the private storage of * the PGconn, accessible via PQhookData(PGconn*). If no * storage is needed, return NULL or set this hook to NULL. */ void *(*initHookData)(const PGconn *conn); /* Invoked on PQreset and PQresetPoll */ void (*connReset)(const PGconn *conn); /* Invoked on PQfinish. */ void (*connDestroy)(const PGconn *conn); /* Invoked on PQgetResult, internally called by all exec funcs */ void *(*resultCreate)(const PGconn *conn, const PGresult *result); /* Invoked on PQcopyResult */ void *(*resultCopy)(PGresult *dest, const PGresult *src); /* Invoked when PQclear is called */ void (*resultDestroy)(const PGresult *result); } PGobjectHooks; -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: Which callback do we use as the key? Currently, none are required (only the name was required). We have to choose one callback that must be provided. What? I thought what you wanted back was the void * pointer that had been registered with a particular callback function. So you use that callback function. If it's not actually registered, you get a NULL. This is what is passed to PQaddObjectHooks, along with a conn: This is all wrong IMHO, not least because it creates ABI problems if you want to add another hook type later. Register each hook separately, eg typedef void (*PGCRHook) (PGconn *conn, void *passthrough); void PQregisterConnResetHook(PGconn *conn, PQCRHook func, void *passthrough); ... repeat for each possible hook ... regards, tom lane One can make a case to break apart the obj hooks structure into individual register functions, but I think you have a different idea in your head than what is being proposed. For starters, there is no passthru pointer to register with a callback (there could be but that is different than hook data...your register looks more like a user_ptr). The passthru pointer, what we call hookData, is allocated with a PGconn (not provided by the user). This is the point of the initHookData callback. typedef void *(*PGinitHookData)(const PGconn *conn); PQregisterInitHookData((PGconn *)NULL, (PGinitHookData)func); PQregisterConnResetHook((PGconn *)NULL, (PGCRHook)func); //etc... conn = PQconnectdb(); When connectdb returns, initHookData has already been called. So, a call to PQhookData(conn, ) will work. BUT, what is still missing from the equation is how to uniquely reference hookData on a conn. What I was previously suggesting was to use the address of initHookData, since w/o this address there wouldn't be any hook data to get. Seemed like a logical choice. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Patch to change psql default banner v6
David Fetter wrote: I hate to bike-shed this even further, but I'd like to make those incompatibility messages just go away by making 8.4's psql (and all those going forward) support every living version of Postgres at the time of their release, so 8.4's psql would be able to talk seamlessly to Postgres 7.4 :) I think you must have been out in the sun too long. Just look at the pg_dump code if you want something of an idea of what this would involve. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: Which callback do we use as the key? Currently, none are required (only the name was required). We have to choose one callback that must be provided. What? I thought what you wanted back was the void * pointer that had been registered with a particular callback function. So you use that callback function. If it's not actually registered, you get a NULL. This is what is passed to PQaddObjectHooks, along with a conn: This is all wrong IMHO, not least because it creates ABI problems if you want to add another hook type later. Register each hook separately, eg typedef void (*PGCRHook) (PGconn *conn, void *passthrough); void PQregisterConnResetHook(PGconn *conn, PQCRHook func, void *passthrough); ... repeat for each possible hook ... regards, tom lane I am starting to think we have not clarified what it is we are trying to do; maybe hook is the wrong terminology. We need to add members to a conn and result, that's pretty much it. To do this, an api user can register callbacks to receive notifications about created/destroyed states of objects. PQhookData is just like PQerrorMessage in that both are public accessor functions to private object data. The difference is that there can be more than one hookData dynamic struct member on a conn/result at a time, unlike errorMessage; thus the need for an additional lookup value when getting hook data (what was hookName). A solution is to only have one function with an eventId, instead of a register function per event. I am playing with using the name 'event' rather than hook. typedef enum { PQEVTID_INITDATA, PQEVTID_CONNRESET, // resultcreate, resultcopy, etc... } PGobjectEventId; typedef void *(*PGobjectEventProc)(PGobjectEventId evtId, ...); int PQregisterObjectEventProc(PGconn*, PGobjectEventProc); // no more key (hookName), use PGobjectEventProc address void *PQeventData(PGconn *, PGobjectEventProc); void *PQresultEventData(PGresult *, PGobjectEventProc); Now there is just one libpq register function and an enum; very resilient to future additions and ABI friendly. The API user will follow a convention of: if you don't care about an event or don't know what it is, just return NULL from the eventProc function (ignore it). The implementation of a PGobjectEventProc would most likely be a switch on the PGobjectEventId with a couple va_arg() calls (which would be very well documented). -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Patch to change psql default banner v6
David Fetter wrote: On Thu, May 15, 2008 at 06:55:31PM -0400, Andrew Dunstan wrote: David Fetter wrote: I hate to bike-shed this even further, but I'd like to make those incompatibility messages just go away by making 8.4's psql (and all those going forward) support every living version of Postgres at the time of their release, so 8.4's psql would be able to talk seamlessly to Postgres 7.4 :) I think you must have been out in the sun too long. One thing I really treasure about working on the Postgres project is frank feedback. :) I know you know me well enough to realise there was an implied smiley ;-) Just look at the pg_dump code if you want something of an idea of what this would involve. Given that each previous version tied backslash commands to some particular chunk of SQL, what would be the problem with either immediately or lazily setting those to the chunks of SQL already present in previous versions? First, this is not a cost free exercise - it increases code complexity enormously. Second, it's not nearly as easy as that: . new commands have been added . postgres features have been added . catalogs have changed Among other things, help and indeed the available command set would have to become server version sensitive. And you would greatly increase the bar for anyone wanting to add a new command - now they (or someone) would have to work out how the command would or might work n versions back, not just with the current dev version. Doing it lazily isn't acceptable - if we promise \command compatibility with previous server versions then we need to deliver it to the maximum extent possible, and if we don't promise it there's no point in doing this. And, as Tom says, it has nothing really to do with this patch. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Patch to change psql default banner v6
Alvaro Herrera wrote: Andrew Dunstan wrote: Second, it's not nearly as easy as that: . new commands have been added . postgres features have been added . catalogs have changed Well, this just means a different piece of SQL needs to be sent for a command depending on the server version, right? It's not like that's tremendously different. The nice thing about most \X commands is that they embed everything they need in a bunch of SQL, and they don't need much else in C code. So it's not all that difficult. And for commands that have been added later, an initial version could just say this server version does not support this command. It would be already a huge improvement. Probably the biggest change would be to support versions that did not have schemas, but I think it would be OK to punt on that. We already stopped supporting 7.2 anyway. Have at it then. Prove me wrong. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Alvaro Herrera wrote: Andrew Dunstan escribió: The thing that is a bit disturbing is that the whole style of this scheme is very different from the fairly simple APIs that the rest of libpq presents. It's going to make libpq look rather odd, I think. I would have felt happier if the authors had been able to come up with a simple scheme to add API calls to export whatever information they needed, rather than using this callback scheme. I'm not sure I understand this point. Remember that this is here to support the libpqtypes library. There doesn't seem to be a way for an API such as you describe to work. That might well be true. The issue then becomes Do we want to add something with this flavor to libpq? I take it Bruce's answer is No, at least until he has seen more evidence of general usefulness. I think we need to make a decision on this before anyone wastes any more time. It should be noted that while this feels slightly foreign, it isn't hugely invasive, unlike the previous effort - it's only a few hundred lines of new code. If we reject this, presumably the authors will have no alternative than to offer libpqtypes as a patch to libpq. ISTM that we're then asking them to climb over a fairly high hurdle. On the one hand we want them to demonstrate that there's demand for their tool and on the other we make it difficult to distribute and deploy. Second, the hook names are compared case insensitively and by linear search. I don't see any justification for using case insensitive names for hooks in a C program, so I think that part should go. And if we expect to keep anything other than trivial numbers of hooks we should look at some sort of binary or hashed search. Keep in mind that the original patch supported a single hook being registered. Perhaps we could dream about having a couple of hooks registered, but turning into hashed search would seem to be overkill, at least for now. (If hooking into libpq is truly successful we can always improve it later -- it's not an exported detail of the API after all.) Right, it was more the case insensitive part that bothered me. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
I'm wondering why the hooks need names at all. AFAICS all that libpq needs to know about a hook is a callback function address and a void * passthrough pointer. In question is: + void * + PQhookData(const PGconn *conn, const char *hookName) Basically, libpqtypes has various functions that take a PGconn that need the private data that is stored in libpq with the connection. PQhookData just does simple linear search and returns the data. [thinks] are you suggesting something like + void * + PQhookData(const PGconn *conn, const void *hookHandle) ? I would have to take a quick look at the code with Andrew C (he'll be in in a bit)...but this might be doable. The hook callback functions allow the hook implementor to receive created/destroyed events about a PGconn and PGresult (PQreset as well). The hook implementor has the option of associating some memory with either. But, that memory pointer is worthless unless there is a way of referencing it at a later time. HookName would not be needed if the libpq hook API only supported a single Object Hook to be registered per conn (or library-wide). It was requested of us to allow a list of hooks per conn. This requries a way of referencing the item. Functions outside the hook callback functions: - PQparamCreate needs libpq-hook-func void *PQhookData(conn, hookName) - PQgetf needs libpq-hook-func void *PQresultHookData(res, hookName) - Also, the void *resultCreate(...) hook callback implementation inside libpqtypes must use PQhookData on the provided conn so it can copy some conn.hookData properties to the result.hookData. The created result.hookData is returned (one can return NULL if no hookData is needed). I have no issue with merlin's idea of a void *handle, but that doesn't really change the concept at all ... just allows someone registering hooks with libpq to use something other than a string. The hookName string idea feels a little more natural and simple. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Merlin Moncure wrote: On Wed, May 14, 2008 at 10:44 AM, Tom Lane [EMAIL PROTECTED] wrote: I'm wondering why the hooks need names at all. AFAICS all that libpq needs to know about a hook is a callback function address and a void * passthrough pointer. For reference...here is what libpqtypes has to do to bind via the hooking interface: http://libpqtypes.esilo.com/browse_source.html?file=hooks.c Are you proposing something substantially different (not my handle suggestion)? How would it work exactly? merlin It is important to see how NON-hook-callback functions in libpqtypes make use of the hook data. PQparamCreate must get a pointer to the conn hook data http://libpqtypes.esilo.com/browse_source.html?file=param.c#line24 PQgetf must get a pointer to the result hook data http://libpqtypes.esilo.com/browse_source.html?file=exec.c#line65 These are NOT hook callbacks. The hook data is NOT isolated to callback functions. It is memory that is publically accessible, outside hook implementations. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq object hooks
Attached is an updated patch. The only change to this patch is that hookNames are now compared with strcmp rather than strcasecmp. The comparisons occur in fe-mics.c (bottom of file) during PQhookData and PQresultHookData. Not sure this needs clarification, but PQcopyResult, PQresultAlloc and PQsetvalue are not part of the object hooks API. They are part of libpq's result management functions (at least that is what I call them). Hook API - PQaddObjectHooks - PQaddGlobalObjectHooks ** CAN BE REMOVED, SEE BELOW - PQhookData - PQresultHookData Result Management (I would put PQmakeEmptyResult here) - PQcopyResult - PQsetvalue - PQresultAlloc (light wrapper to internal pqResultAlloc) PROPOSAL: PQaddGlobalObjectHooks can be removed by handling per-conn and global hook registeration through PQaddObjectHooks. If the provided PGconn is NULL, add hooks to global libpq list: int PQaddObjectHooks(PGconn *conn, PGobjectHooks *hooks) { if(conn == NULL) ;// global hook register else ;// per-conn register } This would make the Object Hooks API 3 functions instead of 4. The same rules apply to global hook registeration, do this from main() before using libpq as the ObjectHooks list is not thread-safe (no locking). NOTE: The patch is called objhooks.patch which is a misleading. The patch is really comprised of the API calls needed to make libpqtypes work. If anyone feels this should be broken into two patches (like objhooks.patch and resmgnt.patch), let us know. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** exports.txt 19 Mar 2008 00:39:33 - 1.19 --- exports.txt 14 May 2008 17:47:59 - *** *** 138,143 --- 138,150 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQcopyResult 142 + PQsetvalue143 + PQresultAlloc 144 + PQaddObjectHooks 145 + PQaddGlobalObjectHooks146 + PQhookData147 + PQresultHookData 148 \ No newline at end of file Index: fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -C6 -r1.357 fe-connect.c *** fe-connect.c31 Mar 2008 02:43:14 - 1.357 --- fe-connect.c14 May 2008 17:47:59 - *** *** 241,253 PQExpBuffer errorMessage); static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); static void default_threadlock(int acquire); - /* global variable because fe-auth.c needs to access it */ pgthreadlock_t pg_g_threadlock = default_threadlock; /* *Connecting to a Database --- 241,252 *** *** 979,990 --- 978,990 * o If your backend wants to use Kerberos authentication then you must *supply both a host name and a host address, otherwise this function *may block on gethostname. * * */ + PostgresPollingStatusType PQconnectPoll(PGconn *conn) { PGresult *res; charsebuf[256]; *** *** 998,1009 --- 998,1010 * We really shouldn't have been polled in these two cases, but we * can handle it. */ case CONNECTION_BAD: return PGRES_POLLING_FAILED; case CONNECTION_OK: + pqInitObjectHooks(conn); return PGRES_POLLING_OK; /* These are reading states */ case CONNECTION_AWAITING_RESPONSE: case CONNECTION_AUTH_OK: { *** *** 1816,1827 --- 1817,1829 conn-next_eo = EnvironmentOptions; return PGRES_POLLING_WRITING; } /* Otherwise, we are open for business! */ conn-status = CONNECTION_OK; + pqInitObjectHooks(conn); return PGRES_POLLING_OK; } case CONNECTION_SETENV: /* *** *** 1848,1859 --- 1850,1862 default: goto
Re: [PATCHES] column level privileges
Tom Lane wrote: I'm not sure where we go from here. Your GSOC student has disappeared, right? Is anyone else willing to take up the patch and work on it? No, he has not disappeared at all. He is going to work on fixing issues and getting the work up to SQL99 level. Your review should help enormously. Stephen, perhaps you would like to work with him. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
David Fetter wrote: Folks, In psql, \dT doesn't show the elements for enums. Please find patch vs. CVS TIP attached which fixes this per the following TODO item: http://archives.postgresql.org/pgsql-hackers/2008-01/msg00826.php I notice that this patch adds an Elements column to the output of \dT, which will only be used by enum types. That seems rather ... cluttered. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: I notice that this patch adds an Elements column to the output of \dT, which will only be used by enum types. That seems rather ... cluttered. But it'll only be in \dT+ anyway, no? Not in this patch. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
David Fetter wrote: On Sun, May 04, 2008 at 07:49:25PM -0400, Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Tom Lane wrote: But it'll only be in \dT+ anyway, no? Not in this patch. Hmmm ... given that a long list of enum members would bloat the output quite a lot, I think I'd vote for putting it in \dT+. Here's one where it's only in \dT+ Yeah. Committed. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Multiline privileges in \z
Brendan Jurd wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Fri, Apr 18, 2008 at 2:37 AM, Tom Lane wrote: The function names in the patch need schema-qualification in case pg_catalog is not first in the search path. Ah, yes. I should have seen that. Thanks Tom. New version attached with schema-qualification. Committed with slight editorialization and a consequent docs change. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [COMMITTERS] pgsql: Sigh ...
Peter Eisentraut wrote: Andrew Dunstan wrote: This patch should fix things for both sets of changes. And it demonstrates pretty much what you need to do for config options for MSVC. Btw., it is quite easily possible to use the autom4te tracing facility to parse the configure.ac file, in case you are interested in generating some of the Windows build code automatically. For example: $ autoconf -t 'AC_ARG_ENABLE:$1' configure.in integer-datetimes nls shared rpath spinlocks debug profiling dtrace segmented-files depend cassert thread-safety thread-safety thread-safety-force largefile Let me know if you want to do something with that. Yeah, maybe. Let's fix the issue pg_config.h.win32 that Tom raised first. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Multiline privileges in \z
Brendan Jurd wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Fri, Apr 18, 2008 at 2:37 AM, Tom Lane wrote: The function names in the patch need schema-qualification in case pg_catalog is not first in the search path. Ah, yes. I should have seen that. Thanks Tom. New version attached with schema-qualification. Wouldn't this expression: pg_catalog.array_to_string(c.relacl, chr(10)) be better expressed as pg_catalog.array_to_string(c.relacl, E'\n') ? Quoted inside a C literal, the backslash would have to be doubled, of course. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [COMMITTERS] pgsql: Sigh ...
Tom Lane wrote: However, all the values are hardcoded, so nothing in it should relate to settings that come from configure, I believe. These should be dealt with in src/tools/msvc/Solution.pm (mostly in GenerateFiles() ). FYI, I'm about to commit changes moving XLOG_BLCKSZ and XLOG_SEG_SIZE into the domain of configurable stuff, too. This patch should fix things for both sets of changes. And it demonstrates pretty much what you need to do for config options for MSVC. If there's no objection I will apply shortly. cheers andrew Index: src/include/pg_config.h.win32 === RCS file: /cvsroot/pgsql/src/include/pg_config.h.win32,v retrieving revision 1.54 diff -c -r1.54 pg_config.h.win32 *** src/include/pg_config.h.win32 2 May 2008 03:41:46 - 1.54 --- src/include/pg_config.h.win32 2 May 2008 22:04:37 - *** *** 37,51 /* The alignment requirement of a `short'. */ #define ALIGNOF_SHORT 2 - /* Size of a disk block --- this also limits the size of a tuple. You can set -it bigger if you need bigger tuples (although TOAST should reduce the need -to have large tuples, since fields can be spread across multiple tuples). -BLCKSZ must be a power of 2. The maximum possible value of BLCKSZ is -currently 2^15 (32768). This is determined by the 15-bit widths of the -lp_off and lp_len fields in ItemIdData (see include/storage/itemid.h). -Changing BLCKSZ requires an initdb. */ - #define BLCKSZ 8192 - /* Define to the default TCP port number on which the server listens and to which clients will try to connect. This can be overridden at run-time, but it's convenient if your clients have the right default compiled in. --- 37,42 *** *** 600,618 your system. */ /* #undef PTHREAD_CREATE_JOINABLE */ - /* RELSEG_SIZE is the maximum number of blocks allowed in one disk file. Thus, -the maximum size of a single file is RELSEG_SIZE * BLCKSZ; relations bigger -than that are divided into multiple files. RELSEG_SIZE * BLCKSZ must be -less than your OS' limit on file size. This is often 2 GB or 4GB in a -32-bit operating system, unless you have large file support enabled. By -default, we make the limit 1 GB to avoid any possible integer-overflow -problems within the OS. A limit smaller than necessary only means we divide -a large relation into more chunks than necessary, so it seems best to err -in the direction of a small limit. A power-of-2 value is recommended to -save a few cycles in md.c, but is not absolutely required. Changing -RELSEG_SIZE requires an initdb. */ - #define RELSEG_SIZE 131072 - /* The size of a `size_t', as computed by sizeof. */ #define SIZEOF_SIZE_T 4 --- 591,596 Index: src/tools/msvc/Solution.pm === RCS file: /cvsroot/pgsql/src/tools/msvc/Solution.pm,v retrieving revision 1.40 diff -c -r1.40 Solution.pm *** src/tools/msvc/Solution.pm 21 Apr 2008 18:37:28 - 1.40 --- src/tools/msvc/Solution.pm 2 May 2008 22:04:39 - *** *** 34,39 --- 34,56 die XML requires both XSLT and ICONV\n; } } + $options-{blocksize} = 8 + unless $options-{blocksize}; # undef or 0 means default + die Bad blocksize $options-{blocksize} + unless grep {$_ == $options-{blocksize}} (1,2,4,8,16,32); + $options-{segsize} = 1 + unless $options-{segsize}; # undef or 0 means default + # only allow segsize 1 for now, as we can't do large files yet in windows + die Bad segsize $options-{segsize} + unless $options-{segsize} == 1; + $options-{wal_blocksize} = 8 + unless $options-{wal_blocksize}; # undef or 0 means default + die Bad wal_blocksize $options-{wal_blocksize} + unless grep {$_ == $options-{wal_blocksize}} (1,2,4,8,16,32,64); + $options-{wal_segsize} = 16 + unless $options-{wal_segsize}; # undef or 0 means default + die Bad wal_segsize $options-{wal_segsize} + unless grep {$_ == $options-{wal_segsize}} (1,2,4,8,16,32,64); return $self; } *** *** 116,122 print O #define USE_LDAP 1\n if ($self-{options}-{ldap}); print O #define HAVE_LIBZ 1\n if ($self-{options}-{zlib}); print O #define USE_SSL 1\n if ($self-{options}-{openssl}); ! print O #define ENABLE_NLS 1\n if ($self-{options}-{nls}); if ($self-{options}-{float4byval}) { --- 133,148 print O #define USE_LDAP 1\n if ($self-{options}-{ldap}); print O #define HAVE_LIBZ 1\n if ($self-{options}-{zlib}); print O #define USE_SSL 1\n if ($self-{options}-{openssl}); ! print O #define ENABLE_NLS 1\n if ($self-{options}-{nls}); ! ! print O #define BLCKSZ ,1024 * $self-{options}-{blocksize},\n; ! print O #define RELSEG_SIZE , ! (1024 / $self-{options}-{blocksize
Re: [PATCHES] [COMMITTERS] pgsql: Sigh ...
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: ! print O #define RELSEG_SIZE , ! (1024 / $self-{options}-{blocksize}) * ! $self-{options}-{segsize} * 1024, \n; This doesn't look quite right; unless the arithmetic is being done in floating point? I had it like this in configure.in: RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024` blocksize is one of (1,2,4,8,16,32) so it should always be a factor of 1024 unless my arithmetic is awry. I did it that way because I dislike expressions with unbracketed mixed operations - they make me think too much. Also it looks like you missed adding segsize to the config.pl comments. That's deliberate - we are currently only allowing a value of 1 here, so I don't see any point in putting it in the sample config file, even as a comment. When we enable other seg sizes we can add it to the sample file. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [COMMITTERS] pgsql: Sigh ...
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Tom Lane wrote: This doesn't look quite right; unless the arithmetic is being done in floating point? I had it like this in configure.in: RELSEG_SIZE=`expr '(' 1024 '*' ${segsize} / ${blocksize} ')' '*' 1024` blocksize is one of (1,2,4,8,16,32) so it should always be a factor of 1024 unless my arithmetic is awry. I did it that way because I dislike expressions with unbracketed mixed operations - they make me think too much. Well, if you dislike the original on style grounds, you should change it to match. Doing the same thing in two different ways in two places isn't good. OK, done. Patch applied with that addition (it was time I deployed autoconf 2.61 anyway). cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix \dT enum in psql
David Fetter wrote: Folks, In psql, \dT doesn't show the elements for enums. Please find patch vs. CVS TIP attached which fixes this per the following TODO item: http://archives.postgresql.org/pgsql-hackers/2008-01/msg00826.php I don't have a particular problem with this patch - indeed the query in it looks eerily familiar :-) However, I'm wondering if we should wait until a possible rework of the mechanics of enums as recently discussed? Or we could put it in and that way it would have to be redone when enums are rejigged. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] column level privileges
(try 2) Here is an updated patch that applies to HEAD. I will update the wiki. (What is the maximum attachment size that -patches will accept?) cheers andrew I wrote: This patch by Golden Lui was his work for the last Google SoC. I was his mentor for the project. I have just realised that he didn't send his final patch to the list. I guess it's too late for the current commit-fest, but it really needs to go on a patch queue (my memory on this was jogged by Tom's recent mention of $Subject). I'm going to see how much bitrot there is and see what changes are necessary to get it to apply. - Here is a README for the whole patch. According to the SQL92 standard, there are four levels in the privilege hierarchy, i.e. database, tablespace, table, and column. Most commercial DBMSs support all the levels, but column-level privilege is hitherto unaddressed in the PostgreSQL, and this patch try to implement it. What this patch have done: 1. The execution of GRANT/REVOKE for column privileges. Now only INSERT/UPDATE/REFERENCES privileges are supported, as SQL92 specified. SELECT privilege is now not supported. This part includes: 1.1 Add a column named 'attrel' in pg_attribute catalog to store column privileges. Now all column privileges are stored, no matter whether they could be implied from table-level privilege. 1.2 Parser for the new kind of GRANT/REVOKE commands. 1.3 Execution of GRANT/REVOKE for column privileges. Corresponding column privileges will be added/removed automatically if no column is specified, as SQL standard specified. 2. Column-level privilege check. Now for UPDATE/INSERT/REFERENCES privilege, privilege check will be done ONLY on column level. Table-level privilege check was done in the function InitPlan. Now in this patch, these three kind of privilege are checked during the parse phase. 2.1 For UPDATE/INSERT commands. Privilege check is done in the function transformUpdateStmt/transformInsertStmt. 2.2 For REFERENCES, privilege check is done in the function ATAddForeignKeyConstraint. This function will be called whenever a foreign key constraint is added, like create table, alter table, etc. 2.3 For COPY command, INSERT privilege is check in the function DoCopy. SELECT command is checked in DoCopy too. 3. While adding a new column to a table using ALTER TABLE command, set appropriate privilege for the new column according to privilege already granted on the table. 4. Allow pg_dump and pg_dumpall to dump in/out column privileges. 5. Add a column named objsubid in pg_shdepend catalog to record ACL dependencies between column and roles. 6. modify the grammar of ECPG to support column level privileges. 7. change psql's \z (\dp) command to support listing column privileges for tables and views. If \z(\dp) is run with a pattern, column privileges are listed after table level privileges. 8. Regression test for column-level privileges. I changed both privileges.sql and expected/privileges.out, so regression check is now all passed. pg_colpriv_version_0.4-20080421.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Text - C string
Tom Lane wrote: Brendan Jurd [EMAIL PROTECTED] writes: If we don't want to meddle with xmltype/bytea/VarChar at all, we'll have to revert those changes, and I'll have to seriously scale back the cleanup patch I was about to post. Still not sure where we stand on the above. To cast, or not to cast? I dunno. I know there was previously some handwaving about representing XML values in some more intelligent fashion than a plain text string, but I have no idea if anyone is likely to do anything about it in the foreseeable future. It seems very unlikely to me. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Suppress compiler warnings on mingw
Applied, Thanks. wiki updated. cheers andrew ITAGAKI Takahiro wrote: Peter Eisentraut [EMAIL PROTECTED] wrote: Then try using %lu and no casts. That should get rid of the warnings the proper way. Ok, I rewrote it to use %lu for format strings. Jeremy Drake [EMAIL PROTECTED] wrote: sizeof(DWORD) is always 4, even on 64-bit windows. sizeof(long) is also always 4. I got it. This change will work on 64-bit windows, because DWORD is defined as 'unsigned long' there, too. We need to support LLP64 compliers in advance, though. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] MSVC build broken with perl 5.10
Zeugswetter Andreas OSB SD wrote: Magnus Hagander wrote: I just tried the MSVC build on a system with ActiveState Perl 5.10, and it doesn't work. Some quick debugging before I downgraded to 5.8 showed that this regexp in Project.pm line 262: my $replace_re = qr{^([^:\n\$]+\.c)\s*:\s*(?:%\s*: )?\$(\([^\)]+\))\/(.*)\/[^\/]+$}; matches things properly using Perl 5.8 in for example src/bin/initdb/Makefile (matches a total of around 10 Makefiles), but in 5.10 it simply does not match anything... Any perl guru out there who can comment on why? ;-) The answer is actually simple, the \n needs the multiline modifier, and thus the m needs to be part of the quote-like operator. The perl doc states: This operator quotes (and possibly compiles) its STRING (it seems 5.8 did not compile, but 5.10 does) I feel that it is rather not a perl bug, and that the modifiers need to be put on the qr{}. I do not quite see why this re needs to be multiline in the first place, but I have not touched that in the attached patch, that is ready to apply. (modification works in perl 5.6, 5.8, 5.10) Thanks, that makes sense. I wonder how it ever worked before. Anyway, patch applied back as far as 8.2. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] libpq object hooks patch
Here is an updated version of the object hooks patch. It now supports a list of hooks for a PGconn, and PGresult. This had to re-introduce the concept of hook name. Being that there is now a list, you need a way to reference an item of that list. Also added PQobjectHooks and PQresultObjectHooks, to get a pointer to the conn or result hook list. PQmakeResult must allow the ability to pass a list of object hooks in. So, PQresultObjectHooks was born. pqtypes doesn't need (at least at this time) PQobjectHooks but leaving it out felt unbalanced. Note: PQhookData and PQresultHookData can be removed. There functionality can be reproduced by an API user issuing PQobjectHooks or PQresultObjectHooks and manually looking for there hook (normaly to get at the hook-data). Basically, an API user would do themselves what PQhookData is doing. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** exports.txt 19 Mar 2008 00:39:33 - 1.19 --- exports.txt 15 Apr 2008 17:18:38 - *** *** 138,143 --- 138,151 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQmakeResult 142 + PQsetvalue143 + PQresultAlloc 144 + PQaddObjectHooks 145 + PQhookData146 + PQresultHookData 147 + PQobjectHooks 148 + PQresultObjectHooks 149 \ No newline at end of file Index: fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -C6 -r1.357 fe-connect.c *** fe-connect.c31 Mar 2008 02:43:14 - 1.357 --- fe-connect.c15 Apr 2008 17:18:39 - *** *** 241,253 PQExpBuffer errorMessage); static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); static void default_threadlock(int acquire); - /* global variable because fe-auth.c needs to access it */ pgthreadlock_t pg_g_threadlock = default_threadlock; /* *Connecting to a Database --- 241,252 *** *** 979,990 --- 978,990 * o If your backend wants to use Kerberos authentication then you must *supply both a host name and a host address, otherwise this function *may block on gethostname. * * */ + PostgresPollingStatusType PQconnectPoll(PGconn *conn) { PGresult *res; charsebuf[256]; *** *** 1875,1887 * the connection structure must be freed). */ conn-status = CONNECTION_BAD; return PGRES_POLLING_FAILED; } - /* * makeEmptyPGconn * - create a PGconn data structure with (as yet) no interesting data */ static PGconn * makeEmptyPGconn(void) --- 1875,1886 *** *** 1970,1981 --- 1969,1998 * release data that is to be held for the life of the PGconn structure. * If a value ought to be cleared/freed during PQreset(), do it there not here. */ static void freePGconn(PGconn *conn) { + int i; + + for(i=0; i conn-objHooksCount; i++) + { + if(conn-objHooks[i].connDestroy) + { + conn-objHooks[i].connDestroy((const PGconn *)conn); + free(conn-objHooks[i].name); + } + } + + if(conn-objHooks) + { + free(conn-objHooks); + conn-objHooks = NULL; + conn-objHooksCount = conn-objHooksSize = 0; + } + if (conn-pghost) free(conn-pghost); if (conn-pghostaddr) free(conn-pghostaddr); if (conn-pgport) free(conn-pgport); *** *** 2152,2164 --- 2169,2189 { if (conn) { closePGconn(conn); if (connectDBStart(conn)) + { + int i; + (void) connectDBComplete(conn); + + for(i=0; i conn-objHooksCount; i++) + if(conn-objHooks[i].connReset) + conn-objHooks[i].connReset((const PGconn *)conn); + } } } /* * PQresetStart: *** *** 2176,2198 return connectDBStart(conn); } return 0
Re: [PATCHES] libpq object hooks patch
Andrew Chernow wrote: Here is an updated version of the object hooks patch. It now supports a list of hooks for a PGconn, and PGresult. This had to re-introduce the concept of hook name. Being that there is now a list, you need a way to reference an item of that list. Also added PQobjectHooks and PQresultObjectHooks, to get a pointer to the conn or result hook list. PQmakeResult must allow the ability to pass a list of object hooks in. So, PQresultObjectHooks was born. pqtypes doesn't need (at least at this time) PQobjectHooks but leaving it out felt unbalanced. Note: PQhookData and PQresultHookData can be removed. There functionality can be reproduced by an API user issuing PQobjectHooks or PQresultObjectHooks and manually looking for there hook (normaly to get at the hook-data). Basically, an API user would do themselves what PQhookData is doing. Made some changes: 1. Removed the hookName argument to PQaddObjectHooks, since its in the supplied PQobjectHooks struct. I think the argument was lingering from a previous patch. 2. Added the ability to install global object hooks: PQaddGlobalObjectHooks(PGobjectHooks*). The header docs for this function warns that it should only be used before calling libpq functions or creating application threads. There are no management functions for global hooks, like get or remove. If you add global object hooks, your stuck with them until process death. 3. There is a new internal pqInitObjectHooks(PGconn *) that installs the global object hooks on new conns in the CONNECTION_OK status. I call this function within PQconnectPoll (3 different locations). This will call PQaddObjectHooks(conn) for each global hook (so global hooks are always at the front of a conn's hook list). pqInitObjectHooks checks to see if conn-objHooksCount 0 and if it is, the request is ignored and the function returns success. This only happens during a PQreset and PQresetPoll. // global PQaddGlobalObjectHooks(libpq_debugger_hook); // per-conn PQaddObjectHooks(conn, session_manager_hook); Since the existing list of object hooks is scanned for duplicate names when adding them, you will never run into duplicate object hooks in a connection (or in the global list). If the two examples above were both called by an application, the per-conn call would fail. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ ? exports.list ? libpq.so.5.2 ? object_hooks.patch Index: exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -C6 -r1.19 exports.txt *** exports.txt 19 Mar 2008 00:39:33 - 1.19 --- exports.txt 16 Apr 2008 01:14:40 - *** *** 138,143 --- 138,152 PQsendDescribePortal 136 lo_truncate 137 PQconnectionUsedPassword 138 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQmakeResult 142 + PQsetvalue143 + PQresultAlloc 144 + PQaddObjectHooks 145 + PQhookData146 + PQresultHookData 147 + PQobjectHooks 148 + PQresultObjectHooks 149 + PQaddGlobalObjectHooks150 \ No newline at end of file Index: fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -C6 -r1.357 fe-connect.c *** fe-connect.c31 Mar 2008 02:43:14 - 1.357 --- fe-connect.c16 Apr 2008 01:14:40 - *** *** 241,253 PQExpBuffer errorMessage); static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); static void default_threadlock(int acquire); - /* global variable because fe-auth.c needs to access it */ pgthreadlock_t pg_g_threadlock = default_threadlock; /* *Connecting to a Database --- 241,252 *** *** 979,990 --- 978,990 * o If your backend wants to use Kerberos authentication then you must *supply both a host name and a host address, otherwise this function *may block on gethostname. * * */ + PostgresPollingStatusType PQconnectPoll(PGconn *conn) { PGresult *res; charsebuf[256]; *** *** 998,1009 --- 998,1010 * We really shouldn't have been polled in these two cases, but we * can handle it. */ case CONNECTION_BAD: return PGRES_POLLING_FAILED; case CONNECTION_OK: + pqInitObjectHooks
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Kind of a long post, but if you take the time to read it we think it accurately clarifies how we interrupt the current objections and how we see this working. NOTE: any references to overhead are in regards to library size, not performance. would be to insert hooks at library _init() time, meaning that the mere linking of libpgtypes Alvaro Herrera wrote: Maybe there's a way we can have libpqtypes adding calls into some hypothetical libpq hooks. So libpqtypes registers its hooks in _init() or some such, and it gets picked up automatically by any app that links to it. the hook name concept Not needed anymore if we do per-conn hooks. I was doing library wide hooks, it felt natural to allow them to be removed (ability not needed per-conn). You can only remove hooks if you have a means of referencing what you want to remove. From that perspective, the names served a purpose - PQremoveObjectHooks(myhook); you've got it holding a process-wide lock Okay, easy change to install per-conn. I was trying to avoid having to set these hooks on every connection. There are some dirty details in regards to locking. Even if you remove the locking from libpq hooks, you still incur a lock at every hook point inside pqtypes. pqtypes has to map a conn and result (we'll call this a pqobj) to pqtypes typeData. Adding a void* to the hook funcs doesn't help because non-hook functions like getf, paramcreate, etc. only have a ptr to a pqobj: PQgetf(res, ..), PQparamCreate(conn, ..). Since the private storage of a pqobj is not directly accessible, you have to either A) map pqobj addresses to typeData in a pqtypes global array that must be locked or B) add two libpq API calls PQtypeData(conn), PQresultTypeData(res). libpgtypes calls PQinitTypes(PGconn *conn) As this stands, it wouldn't work. You need some hook funcptr arguments. Without them, there is no way to communicate with pqtypes. Tom Lane wrote: hooks that could be used by either libpgtypes or something that would like to do something roughly similar I don't think PQinitTypes, private type data per conn/result or the need for PQtypeData(conn), PQresultTypeData(res) (to avoid locking in pqtypes) keeps things in line with this requirement (generic hook api). Has this requirement changed? BTW, Tom was not the only one to suggest generic design. That's why I came up with object hooks - notifications of libpq object states. Best name/concept I can come up with. PQinitTypes(conn) is really PQaddObjectHook(conn, hook) -- addition of the conn argument -- to keep it generic. In the end, the problem is that the wrong tool hooks is being used for pqtypes. Hooks normally allow a completely unrelated library to receive events. I think we are forcing a hook design on to something that is completely related (libpqtypes is an extension of libpq, getvalue and getf are siblings). There is no need for hooks. If we wanted to add PQsetBillingMethod(PQconn*,PQbillingMethod*), then you could make a case for hooks (obviously the billing api doesn't fit). But that is not the case for PQgetf, PQputf, PQparamExec, PQparamSend, The argument against pqtypes being part of libpq was library size overhead. This was verbalized by many people (issues with redhat packaging were also brought up). I never heard a complaint about the 10 API calls we wanted to add. Only that those 10 API calls came at a 50K library bloat cost, and there were no buyers. This brings me back to the dlopen idea. If you want to use pqtypes, PQtypesLoad(). The guts of the library are in libpqtypes.so so the only thing left in libpq are simple functions like below: // libpq PQparamExec(...) { if(libpqtypes-paramExec)//typesLoad issued dlsym calls // we are in libpq, access to private conn storage granted return libpqtypes-paramExec(conn, conn-typeData, ...); return library not loaded: call PQtypesLoad; } // end user #include libpqtypes.h // pqtypes typedefs, includes libpq-fe.h PQtypesLoad(); // call before using libpq res = PQparamExec(conn, param, .); The library size issue is resolved. I never heard any complaints about this approach. Andrew Dunstan said Please make sure that any scheme you have along these lines will work on Windows DLLs too., which didn't sound like a complaint to me. #ifdef WIN32 # define dlopen(libname, flags) LoadLibraryA(libname) # define dlsym(handle, sym) GetProcAddress(handle, sym) #endif Tom also weighed in but he thought I was confused about his hook idea (as the proposed dlopen is completely different): Tom Wrote: This is still 100% backwards. My idea of a libpq hook is something that could be used by libpgtypes *and other things*. What you are proposing is something where the entire API of the supposed add-on is hard-wired into libpq. He is 100% correct, the dlopen idea is 100% backwards from a hook concept. It was not an implementation idea for the hooks concept, it was a different approach
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Merlin Moncure wrote: On Fri, Apr 11, 2008 at 1:47 PM, Andrew Chernow [EMAIL PROTECTED] wrote: Here are the changes to libpq. It adds the ability to register an Object Hook and create a home-grown result. Patch adds 4 functions. We changed the name of PQresultSetFieldValue to PQsetvalue, which better compliments PQgetvalue. If this patch is acceptable, we will move on to making the required changes to pqtypes; some work has already been done. Whoops! One missing thing here...we forgot to make pqResultAlloc pubilc...pqResultAlloc - PQresultAlloc (see discussion in -hackers). Also, we could use pqResultStrdup (or keep it private, in which case we can re-implement in libpqtypes). merlin The connCreate and resultCreate hooks are in the wrong place. I didn't realize this until I started to implement the hook functions in pqtypes. void (*connCreate)(const char *hookName, const PGconn *conn); The requirements for the connCreate hook are that the PGconn is ready for use. I am currently hooking in connectDBStart, which is dead wrong. After some poking around, it looks like the correct place to hook would be in PQconnectPoll ... does this sound correct? There are 3 places PQconnectPoll returns PGRES_POLLING_OK: one is at the top of the function and the other two are further down with We are open for business! comments. Would I be correct to hook in at these 3 places in PQconnectPoll? void (*resultCreate)(const char *hookName, const PGconn *conn, const PGresult *res); The requirements for resultCreate are that the result is fully constructed. I am currently hooked in PQmakeEmptyResult, again dead wrong. Does PQgetResult sound correct? Also, pqtypes is only interested in CONNECTION_OK and successfull results. But, these hooks are available to more than just pqtypes. What should the when to call connCreate and resultCreate policy be? Should the hook only be called when the conn or result was successfull or should the hooks be called for failed connections/commands as well? -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq patch for pqtypes hook api and PGresult creation
Should the hook only be called when the conn or result was successfull or should the hooks be called for failed connections/commands as well? ISTM that the hooks should be called on success and error (doesn't include cases where a NULL conn or result is returned). I think this makes things more useful. If the hooker (pun intended) isn't interested in error results or conns, it can easily ignore them. connCreate: The best solution I found was to hook into PQconnectPoll. This required making the current PQconnectPoll a static named connectPoll and making PQconnectPoll a wrapper to it. The wrapper just calls connectPoll and checks the status for hook points. resultCreate: PQgetResult seemed like the best place. I only notify the hooks if the resultStatus is NOT copyin or copyout. I diff'd fe-connect.c and fe-exec.c against cvs which shows these changes. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -C6 -r1.357 fe-connect.c *** fe-connect.c31 Mar 2008 02:43:14 - 1.357 --- fe-connect.c12 Apr 2008 13:22:30 - *** *** 240,252 static int parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage); static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); static void default_threadlock(int acquire); ! /* global variable because fe-auth.c needs to access it */ pgthreadlock_t pg_g_threadlock = default_threadlock; /* --- 240,252 static int parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage); static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); static void default_threadlock(int acquire); ! static void notifyConnCreateHooks(PGconn *conn); /* global variable because fe-auth.c needs to access it */ pgthreadlock_t pg_g_threadlock = default_threadlock; /* *** *** 891,903 --- 891,907 connectDBComplete(PGconn *conn) { PostgresPollingStatusType flag = PGRES_POLLING_WRITING; time_t finish_time = ((time_t) -1); if (conn == NULL || conn-status == CONNECTION_BAD) + { + if(conn) + notifyConnCreateHooks(conn); return 0; + } /* * Set up a time limit, if connect_timeout isn't zero. */ if (conn-connect_timeout != NULL) { *** *** 979,992 * o If your backend wants to use Kerberos authentication then you must *supply both a host name and a host address, otherwise this function *may block on gethostname. * * */ ! PostgresPollingStatusType ! PQconnectPoll(PGconn *conn) { PGresult *res; charsebuf[256]; if (conn == NULL) return PGRES_POLLING_FAILED; --- 983,997 * o If your backend wants to use Kerberos authentication then you must *supply both a host name and a host address, otherwise this function *may block on gethostname. * * */ ! ! static PostgresPollingStatusType ! connectPoll(PGconn *conn) { PGresult *res; charsebuf[256]; if (conn == NULL) return PGRES_POLLING_FAILED; *** *** 1875,1886 --- 1880,1908 * the connection structure must be freed). */ conn-status = CONNECTION_BAD; return PGRES_POLLING_FAILED; } + /* See connectPoll. All this does is wrap the real PQconnectPoll so + * we can trap PGRES_POLLING_OK or PGRES_POLLING_FAILED statuses. This + * could be done in the real connectPoll, but that requires littering the + * function with notifyConnCreateHooks calls because there are many + * places the function returns a status. This solution is much less + * obtrusive and avoids messing with the black magic of connectPoll. + */ + PostgresPollingStatusType + PQconnectPoll(PGconn *conn) + { + PostgresPollingStatusType status = connectPoll(conn); + + if(status == PGRES_POLLING_OK || status == PGRES_POLLING_FAILED) + notifyConnCreateHooks(conn); + + return status; + } /* * makeEmptyPGconn * - create a PGconn data structure with (as yet) no interesting data */ static PGconn * *** *** 1970,1981 --- 1992,2015 * release data that is to be held for the life of the PGconn structure
[PATCHES] libpq patch for pqtypes hook api and PGresult creation
Here are the changes to libpq. It adds the ability to register an Object Hook and create a home-grown result. Patch adds 4 functions. We changed the name of PQresultSetFieldValue to PQsetvalue, which better compliments PQgetvalue. If this patch is acceptable, we will move on to making the required changes to pqtypes; some work has already been done. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/interfaces/libpq/exports.txt === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.19 diff -c -r1.19 exports.txt *** src/interfaces/libpq/exports.txt19 Mar 2008 00:39:33 - 1.19 --- src/interfaces/libpq/exports.txt11 Apr 2008 17:36:26 - *** *** 141,143 --- 141,147 pg_valid_server_encoding_id 139 PQconnectionNeedsPassword 140 lo_import_with_oid 141 + PQaddObjectHook 142 + PQremoveObjectHook143 + PQmakeResult 144 + PQsetvalue145 \ No newline at end of file Index: src/interfaces/libpq/fe-connect.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.357 diff -c -r1.357 fe-connect.c *** src/interfaces/libpq/fe-connect.c 31 Mar 2008 02:43:14 - 1.357 --- src/interfaces/libpq/fe-connect.c 11 Apr 2008 17:36:26 - *** *** 866,872 --- 866,887 * we are in PGRES_POLLING_WRITING connection state. */ if (PQconnectPoll(conn) == PGRES_POLLING_WRITING) + { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(objHooks)) 0) + { + int i; + + for(i=0; i objHooksCount; i++) + if(objHooks[i].connCreate) + objHooks[i].connCreate(objHooks[i].name, conn); + } + pqReleaseObjectHooks(); + return 1; + } connect_errReturn: if (conn-sock = 0) *** *** 1973,1978 --- 1988,2006 static void freePGconn(PGconn *conn) { + int objHooksCount; + const PGobjectHooks *objHooks; + + if((objHooksCount = pqGetObjectHooks(objHooks)) 0) + { + int i; + + for(i=0; i objHooksCount; i++) + if(objHooks[i].connDestroy) + objHooks[i].connDestroy(objHooks[i].name, conn); + } + pqReleaseObjectHooks(); + if (conn-pghost) free(conn-pghost); if (conn-pghostaddr) Index: src/interfaces/libpq/fe-exec.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.194 diff -c -r1.194 fe-exec.c *** src/interfaces/libpq/fe-exec.c 1 Jan 2008 19:46:00 - 1.194 --- src/interfaces/libpq/fe-exec.c 11 Apr 2008 17:36:27 - *** *** 63,69 static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! /* * Space management for PGresult. --- 63,69 static PGresult *PQexecFinish(PGconn *conn); static int PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target); ! static int check_field_number(const PGresult *res, int field_num); /* * Space management for PGresult. *** *** 139,144 --- 139,146 PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) { PGresult *result; + int objHooksCount; + const PGobjectHooks *objHooks; result = (PGresult *) malloc(sizeof(PGresult)); if (!result) *** *** 192,200 --- 194,341 result-client_encoding = PG_SQL_ASCII; } + if((objHooksCount = pqGetObjectHooks(objHooks)) 0) + { + int i; + + for(i=0; i objHooksCount; i++) + if(objHooks[i].resultCreate) + objHooks[i].resultCreate(objHooks[i].name, conn, result); + } + pqReleaseObjectHooks(); + return result; } + PGresult *PQmakeResult( + PGconn *conn, + int numAttributes, + PGresAttDesc *attDescs) + { + int i; + PGresult *res; + + if(numAttributes = 0 || !attDescs) + return NULL; + + res = PQmakeEmptyPGresult(conn, PGRES_TUPLES_OK); + if(!res) + return NULL; + + res-attDescs = (PGresAttDesc *) + pqResultAlloc(res, numAttributes * sizeof(PGresAttDesc), TRUE); + + if(!res-attDescs
[PATCHES] libpq Win32 Mutex performance patch
I noticed several months ago, and came across it again today, that libpq's pthread-win32.c implementation is using CreateMutex rather than CRITICAL_SECTION. CreateMutex is like a semaphore in that it is designed to be accessible via name system-wide. Even when you don't give it a name, thus bound to process that created it, it still carries significant overhead compared to using win32 CRITICAL_SECTIONs. The attached patch replaces the win32 mutex calls with critical section calls. The change will not affect the behavior of the windows pthread_xxx functions. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/port/pthread-win32.h === RCS file: /projects/cvsroot/pgsql/src/port/pthread-win32.h,v retrieving revision 1.2 diff -c -r1.2 pthread-win32.h *** src/port/pthread-win32.h18 Apr 2007 08:32:40 - 1.2 --- src/port/pthread-win32.h11 Apr 2008 18:35:33 - *** *** 2,8 #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef HANDLE pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); --- 2,8 #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef CRITICAL_SECTION *pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); Index: src/interfaces/libpq/pthread-win32.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/pthread-win32.c,v retrieving revision 1.15 diff -c -r1.15 pthread-win32.c *** src/interfaces/libpq/pthread-win32.c1 Jan 2008 19:46:00 - 1.15 --- src/interfaces/libpq/pthread-win32.c11 Apr 2008 18:35:33 - *** *** 35,51 void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = CreateMutex(0, 0, 0); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! WaitForSingleObject(*mp, INFINITE); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! ReleaseMutex(*mp); } --- 35,69 void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! if(mp) ! { ! *mp = (CRITICAL_SECTION *)malloc(sizeof(CRITICAL_SECTION)); ! if(*mp) ! InitializeCriticalSection(*mp); ! } } void pthread_mutex_lock(pthread_mutex_t *mp) { ! if(mp *mp) ! EnterCriticalSection(*mp); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! if(mp *mp) ! LeaveCriticalSection(*mp); } + + /* If ever needed + pthread_mutex_destroy(pthread_mutex_t *mp) + { + if(mp *mp) + { + DeleteCriticalSection(*mp); + *mp = NULL; + } + } + */ \ No newline at end of file -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq Win32 Mutex performance patch
Magnus Hagander wrote: It changes the behavior when the pointer passed in is invalid from crash to silent working, right? Correct, it a Habit. I sub-consciously write code that checks pointers. We can remove the pointer checks and let the thing dump core if people prefer. Which brings up the second point - is there any actual reason for adding the pthread_mutex_destroy call? Since libpq never calls it, and it's never used from outside libpq (it's not exported outside the library even), isn't it just going to end up as dead code? //Magnus The destroy call is within a comment. I only put it there in case it is ever needeed. BTW, I just noticed the commented destroy call forgot to free(*mp) ... ooppssseee. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq Win32 Mutex performance patch
Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: The attached patch replaces the win32 mutex calls with critical section calls. The change will not affect the behavior of the windows pthread_xxx functions. Why have you defined the lock/unlock functions as willing to fall through silently if handed a null pointer? I think a crash in such a case is what we *want*. Silently not locking is surely not very safe. regards, tom lane Yeah, both naughty. These functions were not implemented to spec. These pthread functions are all supposed to return an int (which is an errno value). I was trying not to change the existing prototypes ... should I? I can return EINVAL if something is NULL and ENOMEM if the malloc fails ... or just dump core. If you like the return value idea, I can make all occurances of pthread_xxx check the return value. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq Win32 Mutex performance patch
Tom Lane wrote: Silently not locking is surely not very safe. Here is the dump code version of the patch. If anyone wants the return value idea, let me know. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ Index: src/port/pthread-win32.h === RCS file: /projects/cvsroot/pgsql/src/port/pthread-win32.h,v retrieving revision 1.2 diff -c -r1.2 pthread-win32.h *** src/port/pthread-win32.h18 Apr 2007 08:32:40 - 1.2 --- src/port/pthread-win32.h11 Apr 2008 19:22:17 - *** *** 2,8 #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef HANDLE pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); --- 2,8 #define __PTHREAD_H typedef ULONG pthread_key_t; ! typedef CRITICAL_SECTION *pthread_mutex_t; typedef int pthread_once_t; DWORD pthread_self(void); Index: src/interfaces/libpq/pthread-win32.c === RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/pthread-win32.c,v retrieving revision 1.15 diff -c -r1.15 pthread-win32.c *** src/interfaces/libpq/pthread-win32.c1 Jan 2008 19:46:00 - 1.15 --- src/interfaces/libpq/pthread-win32.c11 Apr 2008 19:22:17 - *** *** 35,51 void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = CreateMutex(0, 0, 0); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! WaitForSingleObject(*mp, INFINITE); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! ReleaseMutex(*mp); } --- 35,61 void pthread_mutex_init(pthread_mutex_t *mp, void *attr) { ! *mp = (CRITICAL_SECTION *)malloc(sizeof(CRITICAL_SECTION)); ! InitializeCriticalSection(*mp); } void pthread_mutex_lock(pthread_mutex_t *mp) { ! EnterCriticalSection(*mp); } void pthread_mutex_unlock(pthread_mutex_t *mp) { ! LeaveCriticalSection(*mp); } + + /* If ever needed + void pthread_mutex_destroy(pthread_mutex_t *mp) + { + DeleteCriticalSection(*mp); + free(*mp); + *mp = NULL; + } + */ \ No newline at end of file -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq Win32 Mutex performance patch
Andrew Chernow wrote: Tom Lane wrote: Silently not locking is surely not very safe. Here is the dump code version of the patch. If anyone wants the return value idea, let me know. A more graceful solution would be to print something to stderr and then exit. This allows an app's atexit funcs to run. I don't think libpq should core dump an app by choice. I'd be pretty upset if a library I was using core dumped in some cases rather than exiting. andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq Win32 Mutex performance patch
daveg wrote: On Fri, Apr 11, 2008 at 06:25:53PM -0400, Tom Lane wrote: Andrew Chernow [EMAIL PROTECTED] writes: A more graceful solution would be to print something to stderr and then exit. stderr doesn't exist, or point to a useful place, in many environments. And a forced exit() is no better than a crash for most purposes. I don't think libpq should core dump an app by choice. The case that we are talking about is a bug, or would be a bug if it could happen (the fact that we've gotten along happily with no similar test in the existing code shows that it can't). Many forms of bug can result in core dumps; it's foolish to try to prevent them all. And bloating one line of code into five or more lines to defend against can't-happen cases is a good way to end up with unreadable, unmaintainable software. regards, tom lane +1 -dg okay. BTW, my real interest here is the libpq hooks patch requires a lock/unlock for every conn-create, conn-destroy, result-create, result-destroy. Currently, it looks like only the ssl stuff uses mutexes. Adding more mutex use is a good case for a more optimized approach on windows. andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fix for win32 stat() problems
Magnus Hagander wrote: Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Trying to prepare a patch that does it the normal way, but so far I'm failing rather miserably. The *struct* stat is already redefined on win32, so whenever I try #undef or so it conflicts with that :-( Since there is no way to #undef only the parametrized version. I don't follow ... there's no such redefinition in our code AFAICS. Do you mean that the system header files declare it as a macro? Yes. How about #defining safe_stat to be pg_win32_safe_stat on Windows and simply stat elsewhere? Then use safe_stat at the places you consider critical. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: This has been saved for the next commit-fest: http://momjian.postgresql.org/cgi-bin/pgpatches_hold Er, why saved? Until there's a new patch submission there's not going to be more work to do on this in the next fest. I think maybe you need to think a bit harder about the distinction between your TODO-items-in-waiting list and the commit fest queue. I was willing to wade through a pile of TODO-items-in-waiting this time, because I pressed you to make the list available before having sorted through it; but I'm not going to be pleased to see those same threads in the fest queue next time, unless someone's done some actual work in between. It is in the next fest so I will remember to ask if people have done any work on them --- if not they are either deleted or moved to the next commit fest. Are you suggesting we just delete the threads and let them die if they don't submit a new version? My understanding was that all items in a commit-fest have one of these three dispositions: . committed . rejected . referred back to author for more work We're really only interested in the third one here, and so, yes, the ball should be in the author's court, not yours. I don't see any reason for you to move items from one queue to another like that. It just looks like it's making work. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Headers dependencies cleanup
Zdenek Kotala wrote: Is it your assumption or do you mean some specific compiler? IIRC, inline is defined in C99 and my assumption :-) is that it should be supported by all compilers today. I try to look on buildmachine, There should be some useful configure output. My recollection is that we support C89, which might be a bit out of date, but then we try not to obsolete platforms if possible. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq type system 0.9a
Joe Conway wrote: Alvaro Herrera wrote: Merlin Moncure escribió: Yesterday, we notified -hackers of the latest version of the libpq type system. Just to be sure the right people are getting notified, we are posting the latest patch here as well. Would love to get some feedback on this. I had a look at this patch some days ago, and the first question in my mind was: why is it explicitely on libpq? Why not have it as a separate library (say libpqtypes)? That way, applications not using it would not need to link to it. Applications interested in using it would just need to add another -l switch to their link line. +1 Joe What is gained by having a separate library? Our changes don't bloat the library size so I'm curious what the benefits are to not linking with it? If someone doesn't want to use, they don't have to. Similar to the backend, there is stuff in there I personally don't use (like geo types), but I'm not sure that justifies a link option -lgeotypes. The changes we made are closely tied to libpq's functionality. Adding PQputf to simplify the parameterized API, adding PQgetf to compliment PQgetvalue and added the ability to register user-defined type handlers (used by putf and getf). PQgetf makes extensive use of PGresult's internal API, especially for arrays and composites. Breaking this into a separate library would require an external library to access the private internals of libpq. Personally, I am not really in favor of this idea because it breaks apart code that is very related. Although, it is doable. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] libpq type system 0.9a
Andrew Chernow wrote: Joe Conway wrote: Alvaro Herrera wrote: Merlin Moncure escribió: Yesterday, we notified -hackers of the latest version of the libpq type system. Just to be sure the right people are getting notified, we are posting the latest patch here as well. Would love to get some feedback on this. I had a look at this patch some days ago, and the first question in my mind was: why is it explicitely on libpq? Why not have it as a separate library (say libpqtypes)? That way, applications not using it would not need to link to it. Applications interested in using it would just need to add another -l switch to their link line. +1 Joe What is gained by having a separate library? Our changes don't bloat the library size so I'm curious what the benefits are to not linking with it? If someone doesn't want to use, they don't have to. Similar to the backend, there is stuff in there I personally don't use (like geo types), but I'm not sure that justifies a link option -lgeotypes. The changes we made are closely tied to libpq's functionality. Adding PQputf to simplify the parameterized API, adding PQgetf to compliment PQgetvalue and added the ability to register user-defined type handlers (used by putf and getf). PQgetf makes extensive use of PGresult's internal API, especially for arrays and composites. Breaking this into a separate library would require an external library to access the private internals of libpq. Personally, I am not really in favor of this idea because it breaks apart code that is very related. Although, it is doable. I poked around to see how this would work. There are some problems. 1. members were added to PGconn so connection-based type handler information can be copied to PGparam and PGresult objects. 2. members were added to PGresult, referenced in #1. To properly getf values, the connection-based type handler information must be available to PGresult. Otherwise, PQgetf would require an additional argument, a PGconn, which may have been closed already. 3. PQfinish calls pqClearTypeHandler to free type info assigned to the PGconn. 4. PQclear also calls pqClearTypeHandlers It would also remove some of the simplicity. Creating a connection would no longer initialized type info, which gets copied to PGparam and PGresult. Type info includes a list of built-in handlers and backend config, like integer_datetimes, server-version, etc... That means an additional function must be called after PQconnectdb. But where would the type info be stored? It wouldn't exist in PGconn anymore? Also, this would require double frees. You have to free the result as well as the type info since they are no longer one object. Same holds true for a pgconn. There is something elegant about not requiring additional API calls to perform a putf or getf. It'll just work if you want to use it. You can use PQgetf on a result returned by PQexec and you can use PQputf, PQparamExec followed by PQgetvalue. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Expose checkpoint start/finish times into SQL.
Joshua D. Drake wrote: Theo Schlossnagle wrote: First whack at exposing the start and finish checkpoint times into SQL. Why is that useful? For knowing how long checkpoints are taking. If they are taking too long you may need to adjust your bgwriter settings, and it is a serious drag to parse postgresql logs for this info. Even if this were true, surely the answer is to improve the logging. Has this feature been discussed on -hackers? I don't recall it (and my memory has plenty of holes in it), but I'm sure that after attending my talk last Sunday Theo hasn't sent in a patch for an undiscussed feature ;-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Expose checkpoint start/finish times into SQL.
Robert Treat wrote: On Thursday 03 April 2008 19:08, Andrew Dunstan wrote: Joshua D. Drake wrote: Theo Schlossnagle wrote: First whack at exposing the start and finish checkpoint times into SQL. Why is that useful? For knowing how long checkpoints are taking. If they are taking too long you may need to adjust your bgwriter settings, and it is a serious drag to parse postgresql logs for this info. Even if this were true, surely the answer is to improve the logging. Exposing everything into the log files isn't always sufficient (says the guy who maintains a remote admin tool) It should be now that you can have machine readable logs (says the guy who literally spent weeks making that happen) ;-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Expose checkpoint start/finish times into SQL.
Joshua D. Drake wrote: Exposing everything into the log files isn't always sufficient (says the guy who maintains a remote admin tool) It should be now that you can have machine readable logs (says the guy who literally spent weeks making that happen) ;-) And how does the person get access to those? And what script do I need to write to make it happen? Don't get me wrong, the feature you worked entirely too hard on to get working is valuable but... being able to say, SELECT * FROM give_me_my_db_info; is much more useful in this context. How to load the CSV logs is very clearly documented. It's really *very* easy, so easy it's mostly CP. See http://www.postgresql.org/docs/current/static/runtime-config-logging.html#RUNTIME-CONFIG-LOGGING-CSVLOG If you are trying to tell me that that's too hard for a DBA, then I have to say you need better DBAs. In short, I should never have to go to log for this class of information. It should be available in the database. What you haven't explained is why this information needs to be kept in the db on a historical basis, as opposed to all the other possible diagnostics where history might be useful (and, as Tom has pointed out, this patch doesn keep it historically any way). I think there is quite possibly a good case for keeping some diagnostics in a table or tables, on a rolling basis, maybe. But then that's a facility that needs to be properly designed. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Expose checkpoint start/finish times into SQL.
Theo Schlossnagle wrote: Has this feature been discussed on -hackers? I don't recall it (and my memory has plenty of holes in it), but I'm sure that after attending my talk last Sunday Theo hasn't sent in a patch for an undiscussed feature ;-) Andrew: I don't think this feature has been discussed on hackers. The patch took about 15 minutes to author, so it sounds like the most concise way to start a conversation. Seems silly to start the conversation on hackers with a patch. :-) Well, not really. I believe -hackers has a much larger readership than -patches, so even for small features we generally want them discussed there. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] actualized SQL/PSM patch
Jonah H. Harris wrote: On Tue, Apr 1, 2008 at 5:55 PM, Tom Lane [EMAIL PROTECTED] wrote: The fundamental problem I've got with this patch is that it adds 400K of new code (and that's just the code, not counting documentation or regression tests) that we'll have to maintain, to obtain a feature that so far as I've heard there is precisely zero demand for. We have a customer that wants to use it as part of a MySQL-to-Postgres migration. Using an implementation like this? I suspect anyone wanting to migrate their existing SQL/PSM stuff to Postgres will be less than impressed by our function body as a string mechanism. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Consistent \d commands in psql
Tom Lane wrote: One question: should \df really list *all* nonsystem functions? Or just the ones that are visible in your search path? I'd be inclined to say the second. +1 (although maybe that discussion belongs on -hackers, or even -general) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Fwd: WIP: CASE statement for PL/pgSQL
Pavel Stehule wrote: correct queue Hello I finished this patch. Proposal: http://archives.postgresql.org/pgsql-hackers/2008-01/msg00696.php It's compatible with PL/SQL (Oracle) and SQL/PSM (ANSI). At the very least this patch is missing documentation and regression tests. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] create language ... if not exists
Heikki Linnakangas wrote: Andreas 'ads' Scherbaum wrote: The attached patch for HEAD extends the CREATE LANGUAGE statement by an IF NOT EXISTS option which in effect changes the raised error into a notice. Before i continue working on this patch i would like to know if this extension has a chance to go into PG and what other changes i should apply (beside the missing documentation). The way we've solved this problem for other CREATE commands is to add OR REPLACE option, instead of IF NOT EXISTS. We should do the same here. My recollection is that we only do that where we need to for reasons of dependency. Not sure that applies here. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Include Lists for Text Search
Simon Riggs wrote: As Greg mentions on another thread, not all patches are *intended* to be production quality by their authors. Many patches are shared for the purpose of eliciting general feedback. You yourself encourage a group development approach and specifically punish those people dropping completely finished code into the queue and expecting it to be committed as-is. If you post a patch that is not intended to be of production quality, it is best to mark it so explicitly. Then nobody can point fingers at you. Also, Bruce would then know not to put it in the queue of patches waiting for application. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [HACKERS] Include Lists for Text Search
Simon Riggs wrote: On Mon, 2008-03-10 at 08:24 -0400, Andrew Dunstan wrote: Simon Riggs wrote: As Greg mentions on another thread, not all patches are *intended* to be production quality by their authors. Many patches are shared for the purpose of eliciting general feedback. You yourself encourage a group development approach and specifically punish those people dropping completely finished code into the queue and expecting it to be committed as-is. If you post a patch that is not intended to be of production quality, it is best to mark it so explicitly. Then nobody can point fingers at you. Also, Bruce would then know not to put it in the queue of patches waiting for application. So it can be forgotten about entirely? H. I think if you post something marked Work In Progress, there is an implied commitment on your part to post something complete at a later stage. So if it's forgotten you would be the one doing the forgetting. ;-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: Andrew Dunstan wrote: Another question that occurred to me - did you try using strpbrk() to look for the next interesting character rather than your homegrown searcher gadget? If so, how did that perform? It looks like strpbrk() performs poorly: Yes, not surprising. I just looked at the implementation in glibc, which I assume you are using, and it seemed rather basic. The one in NetBSD's libc looks much more efficient. See http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/libc/string/strpbrk.c?rev=1.1.2.1content-type=text/plaincvsroot=glibc and http://cvsweb.de.netbsd.org/cgi-bin/cvsweb.cgi/src/lib/libc/string/strpbrk.c?rev=1.16;content-type=text%2Fx-cvsweb-markup Not that what you've done isn't good, if a little obscure (as is the NetBSD implementation) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: Andrew Dunstan wrote: I'm still a bit worried about applying it unless it gets some adaptive behaviour or something so that we don't cause any serious performance regressions in some cases. I'll try to come up with something. At the most conservative end, we could fall back to the current method on the first escape, quote or backslash character. Also, could we perhaps benefit from inlining some calls, or is your compiler doing that anyway? gcc does inline all static functions that are only called from one site, and small functions, using some heuristic. I don't think more aggressive inlining would help. Another question that occurred to me - did you try using strpbrk() to look for the next interesting character rather than your homegrown searcher gadget? If so, how did that perform? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] CopyReadAttributesCSV optimization
Heikki Linnakangas wrote: Here's a patch to speed up CopyReadAttributesCSV. On the test case I've been playing with, loading the TPC-H partsupp table, about 20% CopyReadAttributesCSV (inlined into DoCopy, DoCopy itself is insignificant): [snip] The trick is to split the loop in CopyReadAttributesCSV into two parts, inside quotes, and outside quotes, saving some instructions in both parts. Your mileage may vary, but I'm quite happy with this. I haven't tested it much yet, but I wouldn't expect it to be a loss in any interesting scenario. The code also doesn't look much worse after the patch, perhaps even better. This looks sane enough, and worked for me in testing, so I'm going to apply it shortly. I'll probably add a comment or two about how the loops interact. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: Heikki Linnakangas wrote: Heikki Linnakangas wrote: Attached is a patch that modifies CopyReadLineText so that it uses memchr to speed up the scan. The nice thing about memchr is that we can take advantage of any clever optimizations that might be in libc or compiler. Here's an updated version of the patch. The principle is the same, but the same optimization is now used for CSV input as well, and there's more comments. Another update attached: It occurred to me that the memchr approach is only safe for server encodings, where the non-first bytes of a multi-byte character always have the hi-bit set. We currently make the following assumption in the code: * These four characters, and the CSV escape and quote characters, are * assumed the same in frontend and backend encodings. * The four characters are the carriage return, line feed, backslash and dot. I think the requirement might well actually be somewhat stronger than that: i.e. that none of these will appear as a non-first byte in any multi-byte client encoding character. If that's right, then we should be able to write CopyReadLineText without bothering about multi-byte chars. If it's not right then I suspect we have some cases that can fail now anyway. (I believe some client encodings at least use backslash in subsequent chars, and that's a nasty one because the \. end sequence is hard coded). I believe all the chars up to 0x2f are safe - that includes both quote chars and dot) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Tom Lane wrote: BTW, I notice that the code allows CSV escape and quote characters that have the high bit set (in single-byte server encodings that is). Is this a good idea? It seems like such are extremely unlikely to be the same in two different encodings. Maybe we should restrict to the ASCII range? Especially if the client encoding is multibyte ... In the commonest case these are both either or '. I would not have any objection to requiring them to be ASCII chars. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: Andrew Dunstan wrote: Heikki Linnakangas wrote: Another update attached: It occurred to me that the memchr approach is only safe for server encodings, where the non-first bytes of a multi-byte character always have the hi-bit set. We currently make the following assumption in the code: * These four characters, and the CSV escape and quote characters, are * assumed the same in frontend and backend encodings. * The four characters are the carriage return, line feed, backslash and dot. I think the requirement might well actually be somewhat stronger than that: i.e. that none of these will appear as a non-first byte in any multi-byte client encoding character. If that's right, then we should be able to write CopyReadLineText without bothering about multi-byte chars. If it's not right then I suspect we have some cases that can fail now anyway. No, we don't require that, and we do handle it correctly. We use pg_encoding_mblen to determine the length of each character in CopyReadLineText when the encoding is a client-only encoding, and only look at the first byte of each character. In CopyReadAttributesText, where we have a similar loop, we've already transformed the input to server encoding. Oops. I see that now. Funny how I missed it when I went looking for it :-( I think I understand the patch now :-) I'm still a bit worried about applying it unless it gets some adaptive behaviour or something so that we don't cause any serious performance regressions in some cases. Also, could we perhaps benefit from inlining some calls, or is your compiler doing that anyway? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: Andrew Dunstan wrote: I'm still a bit worried about applying it unless it gets some adaptive behaviour or something so that we don't cause any serious performance regressions in some cases. I'll try to come up with something. At the most conservative end, we could fall back to the current method on the first escape, quote or backslash character. That's far too conservative, I think. Somewhere a bit short of your observed breakeven point seems about right. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Greg Smith wrote: On Thu, 6 Mar 2008, Heikki Linnakangas wrote: At the most conservative end, we could fall back to the current method on the first escape, quote or backslash character. I would just count the number of escaped/quote characters on each line, and then at the end of the line switch modes between the current code on the new version based on what the previous line looked like. That way the only additional overhead is a small bit only when escapes show up often, plus a touch more just once per line. Barely noticable in the case where nothing is escaped, very small regression for escape-heavy stuff but certainly better than the drop you reported in the last rev of this patch. Rev two of that design would keep a weighted moving average of the total number of escaped characters per line (say wma=(7*wma+current)/8) and switch modes based on that instead of the previous one. There's enough play in the transition between where the two approaches work better at that this should be easy enough to get a decent transition between. Based on your data I would put the transition at wma4, which should keep the old code in play even if only half the lines have the bad regression that shows up with 8 escapes per line. I'd be inclined just to look at the first buffer of data we read in, and make a one-off decision there, if we can get away with it. Then the cost of testing is fixed rather than per line. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] WIP: guc enums
Tom Lane wrote: Having to have two extra hook functions for every variable seems like a lot of notational overhead for not much gain. +1 cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] CopyReadLineText optimization
Heikki Linnakangas wrote: So the overhead of using memchr slows us down if there's a lot of escape or quote characters. The breakeven point seems to be about 1 in 8 characters. I'm not sure if that's a good tradeoff or not... How about we test the first buffer read in from the file and change strategy accordingly? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] NetBSD/MIPS supports dlopen
Alvaro Herrera wrote: Both done -- I backpatched all the way down to 7.4 (and later I noticed that the only 7.3 BF members are NetBSD). Haven't we declared 7.3 at EOL anyway? cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] NetBSD/MIPS supports dlopen
Alvaro Herrera wrote: Andrew Dunstan wrote: Alvaro Herrera wrote: Both done -- I backpatched all the way down to 7.4 (and later I noticed that the only 7.3 BF members are NetBSD). Haven't we declared 7.3 at EOL anyway? That's why I didn't backpatch it there. But if that's the case, why are we still reporting 7.3 in the buildfarm status page? Because until a couple of weeks ago those two machines were still reporting that branch. When they are 30 days old the reports will drop off the page. (It looks like salamander has stopped altogether, which Tom mentioned the other day would distress him some.) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] libpq type system 0.9a
Merlin Moncure wrote: On Wed, Mar 5, 2008 at 5:47 PM, Florian G. Pflug [EMAIL PROTECTED] wrote: Merlin Moncure wrote: Yesterday, we notified -hackers of the latest version of the libpq type system. Just to be sure the right people are getting notified, we are posting the latest patch here as well. Would love to get some feedback on this. Sorry if this has been discussed before, but why is it necessary to specify the type when calling PQgetf on a result? It seems that this formatting string *always* has to match the type list of your select statement, no? yes...it always has to match. the format string requirements could in theory be relaxed (for 'get') but this would break symmetry with 'put' and you would lose a sanity check...getf like scanf writes directly into application memory so the double-specifying (directly in the format string and indirectly in the query) isn't necessarily a bad thing. imagine if your application was 'select * from table' and one of the field types changed...disaster. merlin A few other reasons why is it necessary to specify the type when calling PQgetf on a result Unlike PQgetvalue, all values returned by PQgetf are either native C types or structures ... not C strings. When you call getf you must tell it what types to read out of the result object. Like scanf, they must be the correctly sized data types. PGdate date; int i4; PQgetf(result, tup_num, %date %int4, 0, date, 1, i4); Specifying anything other than a %date or %int4 in the above example is a programming error. You would be asking to fetch a value of the wrong type. Without the formatting string, libpq would have to va_arg(ASSUME_T) your value. // no specifier int i; PQgetf(result, tup, field, i); In the above, libpq would have to use PQftype to determine what the native C type is of your variable argument. If PQftype returned INT8OID, you begin to clobber your application's memory space ... va_arg(ap, long long) on a 32-bit value. This problem is solved by telling libpq what data type you want from a field. Also, the libpq type system enforces strict type checking when performing getf calls. This protects from mis-matches programming errors on types: For example: -- create table t (a int8); PQresult *result = PQexec(conn, SELECT a FROM t); char *val = PQgetvalue(result, ...); int a = atoi(val); // assumed its an int4 In the above example, the libpq user thinks the 'a' column of the 't' table is an int4 when in fact its an int8. The above may work most of the time but will eventually truncate the value and nip you in the butt. With PQgetf, you would get an error saying the server returned an int8 and you are asking for an int4. Thus, the programming bug would be squashed immediately. Also, user-defined types are not known to libpq so PQftype would not really work. They could if the libpq type system referenced data types by OID, but this is not portable to other servers. It is more portable to use the type name. For example, a company with 15 postgresql servers that use the same collection of company-specific user-defined data types. The type names would be the same across the 15 servers but there is no guarentee the OIDs would be. Composites and arrays caused a few issues as well. We also tried to provide as much protection as possible ... in the spirit of the backend. -- Andrew Chernow eSilo, LLC every bit counts http://www.esilo.com/ -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches
Re: [PATCHES] [BUGS] BUG #4007: chr(0) doesn't work anymore
Bruce Momjian wrote: Tom Lane wrote: Steve Clark [EMAIL PROTECTED] writes: I'm not sure I understand what you mean about TEXT being null-safe. What are the issues, and why was it supported for years and now abruptly changed. It never was supported, we are simply plugging a hole that let you create a text value that would be likely to malfunction in subsequent use. Seems we never documented that chr(0) is not supported. I have applied the following doc patch to CVS HEAD and 8.3.X. The NULL (0) character is not allowed because text data types cannot reliably store such bytes. Reliably is arguably misleading here. cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your Subscription: http://mail.postgresql.org/mj/mj_wwwusr?domain=postgresql.orgextra=pgsql-patches