Re: [PATCHES] Reference by output in : \d table_name
On Mon, Mar 31, 2008 at 3:50 AM, Tom Lane [EMAIL PROTECTED] wrote: kenneth d'souza [EMAIL PROTECTED] writes: With reference to the post http://archives.postgresql.org/pgsql-patches/2008-02/msg00104.phpand as stated by -hackers and -patchers, I am submitting the diff -c output as an attachment. Thanks, Kenneth Applied with some revisions. While working on my printTable patch, I noticed that this patch only has an indent of two spaces for incoming foreign keys, while all the other table footers have an indent of four spaces. Was this deliberate? And if so, why the change in indentation for this particular listing? Cheers, BJ -- 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] printTable API (was: Show INHERIT in \du)
On Sun, Mar 30, 2008 at 9:39 AM, Brendan Jurd [EMAIL PROTECTED] wrote: On 25/03/2008, Tom Lane [EMAIL PROTECTED] wrote: Brendan Jurd [EMAIL PROTECTED] writes: This makes me wonder whether print.c could offer something a bit more helpful to callers wishing to DIY a table; we could have a table-building struct with methods like addHeader and addCell. Once you have two occurrences of a pattern, it's reasonable to assume there will be more later. +1 for building a little bit of infrastructure. Per the above discussion, I'm looking at creating a nice API in print.c for callers wishing to build their own psql output table. Currently, if you want to build your own table you need to implement your own local version of the logic in printQuery. describeOneTableDetails is the only place this happens right now, but due to some localisation issues it looks like my \du patch will have to build its table manually as well. I'd like to submit my first version of this patch for review. I have introduced a new struct in print.h called printTableContent, which is used to compose the contents of a psql table. The methods exposed for this struct are as follows: void printTableInit(printTableContent *const content, const char *title, const int ncolumns, const int nrows); void printTableAddHeader(printTableContent *const content, const char *header, const int encoding, const bool translate, const char align); void printTableAddCell(printTableContent *const content, const char *cell, const int encoding, const bool translate); void printTableAddFooter(printTableContent *const content, const char *footer); void printTableSetFooter(printTableContent *const content, const char *footer); void printTableCleanUp(printTableContent *const content); -= Notes The column headers and cells are implemented as simple arrays of pointers to existing strings. It's necessary for the calling site to ensure that these strings survive at least until the table has been printed. That's usually not a problem since the headers and cells tend to be inside a PGresult. Footers are a bit different. I've implemented them as a very primitive singly-linked list which copies its contents with strdup. A lot of the complexity in the pre-patch describeOneTableDetails comes from the fact that it needs to know the number of footers in advance. The singly-linked list allows callers to incrementally add an arbitrary number of footers, and doesn't require them to preserve the strings, which is nice when you're working with a temporary PQExpBuffer to produce a footer. -= QA Not having written much C, I'm concerned about memory management errors. But I've run my own tests with describeOneTableDetails on tables with every kind of footer there is, and put the patch through valgrind, and I haven't encountered any segfaults or memory leaks. Both the serial and parallel regression tests passed perfectly. -= Documentation None required as far as I can tell, aside from code comments. -= Patch tracking I'll add this to the May CommitFest wiki page myself -- Bruce, you don't need to do anything at all! =) Looking forward to your comments. Cheers, BJ print-table.diff_0.bz2 Description: BZip2 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] 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.
* If a
Re: [PATCHES] [HACKERS] Terminating a backend
Bruce Momjian wrote: I have an idea for this TODO item: * Allow administrators to safely terminate individual sessions either via an SQL function or SIGTERM Lock table corruption following SIGTERM of an individual backend has been reported in 8.0. A possible cause was fixed in 8.1, but it is unknown whether other problems exist. This item mostly requires additional testing rather than of writing any new code. http://archives.postgresql.org/pgsql-hackers/2006-08/msg00174.php When we get the termination signal, why can't we just set a global boolean, do a query cancel, and in the setjmp() code block check the global and exit --- at that stage we know we have released all locks and can exit cleanly. I have implemented this idea with the attached patch. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/func.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v retrieving revision 1.429 diff -c -c -r1.429 func.sgml *** doc/src/sgml/func.sgml 10 Apr 2008 13:34:33 - 1.429 --- doc/src/sgml/func.sgml 12 Apr 2008 13:48:33 - *** *** 11849,11854 --- 11849,11857 primarypg_cancel_backend/primary /indexterm indexterm + primarypg_terminate_backend/primary +/indexterm +indexterm primarypg_reload_conf/primary /indexterm indexterm *** *** 11885,11890 --- 11888,11900 /row row entry + literalfunctionpg_terminate_backend/function(parameterpid/parameter typeint/)/literal + /entry +entrytypeboolean/type/entry +entryTerminate a backend/entry + /row + row +entry literalfunctionpg_reload_conf/function()/literal /entry entrytypeboolean/type/entry *** *** 11909,11915 para functionpg_cancel_backend/ sends a query cancel (systemitemSIGINT/) signal to a backend process identified by ! process ID. The process ID of an active backend can be found from the structfieldprocpid/structfield column in the structnamepg_stat_activity/structname view, or by listing the commandpostgres/command processes on the server with --- 11919,11927 para functionpg_cancel_backend/ sends a query cancel (systemitemSIGINT/) signal to a backend process identified by ! process ID. functionpg_terminate_backend/ sends a terminate ! (systemitemSIGTERM/) signal to the specified process ID. The ! process ID of an active backend can be found from the structfieldprocpid/structfield column in the structnamepg_stat_activity/structname view, or by listing the commandpostgres/command processes on the server with Index: src/backend/port/ipc_test.c === RCS file: /cvsroot/pgsql/src/backend/port/ipc_test.c,v retrieving revision 1.24 diff -c -c -r1.24 ipc_test.c *** src/backend/port/ipc_test.c 24 Mar 2008 18:08:47 - 1.24 --- src/backend/port/ipc_test.c 12 Apr 2008 13:48:33 - *** *** 40,45 --- 40,46 volatile bool InterruptPending = false; volatile bool QueryCancelPending = false; + volatile bool BackendTerminatePending = false; volatile bool ProcDiePending = false; volatile bool ImmediateInterruptOK = false; volatile uint32 InterruptHoldoffCount = 0; Index: src/backend/postmaster/autovacuum.c === RCS file: /cvsroot/pgsql/src/backend/postmaster/autovacuum.c,v retrieving revision 1.76 diff -c -c -r1.76 autovacuum.c *** src/backend/postmaster/autovacuum.c 26 Mar 2008 21:10:38 - 1.76 --- src/backend/postmaster/autovacuum.c 12 Apr 2008 13:48:33 - *** *** 1475,1481 * means abort and exit cleanly, and SIGQUIT means abandon ship. */ pqsignal(SIGINT, StatementCancelHandler); ! pqsignal(SIGTERM, die); pqsignal(SIGQUIT, quickdie); pqsignal(SIGALRM, handle_sig_alarm); --- 1475,1481 * means abort and exit cleanly, and SIGQUIT means abandon ship. */ pqsignal(SIGINT, StatementCancelHandler); ! pqsignal(SIGTERM, BackendTerminateHandler); pqsignal(SIGQUIT, quickdie); pqsignal(SIGALRM, handle_sig_alarm); Index: src/backend/tcop/postgres.c === RCS file: /cvsroot/pgsql/src/backend/tcop/postgres.c,v retrieving revision 1.548 diff -c -c -r1.548 postgres.c *** src/backend/tcop/postgres.c 2 Apr 2008 18:31:50 - 1.548 --- src/backend/tcop/postgres.c 12 Apr 2008 13:48:33 - *** *** 2541,2547 * waiting for input, however. */ if
Re: [PATCHES] [HACKERS] Terminating a backend
Bruce Momjian wrote: Bruce Momjian wrote: I have an idea for this TODO item: * Allow administrators to safely terminate individual sessions either via an SQL function or SIGTERM Lock table corruption following SIGTERM of an individual backend has been reported in 8.0. A possible cause was fixed in 8.1, but it is unknown whether other problems exist. This item mostly requires additional testing rather than of writing any new code. http://archives.postgresql.org/pgsql-hackers/2006-08/msg00174.php When we get the termination signal, why can't we just set a global boolean, do a query cancel, and in the setjmp() code block check the global and exit --- at that stage we know we have released all locks and can exit cleanly. I have implemented this idea with the attached patch. One problem I have with my patch is that SIGTERM is currently used by the postmaster to shut down backends. Now because the postmaster knows that all backend are terminating, it can accept a dirtier shutdown than one where we are terminating just one backend and the rest are going to keep running. The new SIGTERM coding is going to exit a backend only in a place where cancel is checked. I need some thoughts in this area. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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
Andrew Chernow [EMAIL PROTECTED] writes: The requirements for the connCreate hook are that the PGconn is ready for use. I am currently hooking in connectDBStart, which is dead wrong. I looked at the object hooks patch and it looked like a complete mess. AFAICS the only way you could use it would be to insert hooks at library _init() time, meaning that the mere linking of libpgtypes into an executable would cause all your hook overhead to occur on every connection and every query ever made by that program. The thread locking you put in is completely horrid as well --- you've got it holding a process-wide lock over operations that are likely to include nontrivial database interactions. I think you need to consider something a bit less invasive. What I'd imagine is something more like this: a program that wishes to use libpgtypes calls PQinitTypes(PGconn *conn) immediately after establishing a connection, and that installs hooks into connection-local storage and does whatever per-connection setup it needs. No need for any global state nor any thread mutexes. Lastly, as far as the hook designs themselves: the hook name concept seems utterly useless, and what *is* needed is missing: every callback function needs a pass-through void * pointer so that it can get at private state. regards, tom lane -- 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] Terminating a backend
Bruce Momjian [EMAIL PROTECTED] writes: When we get the termination signal, why can't we just set a global boolean, do a query cancel, and in the setjmp() code block check the global and exit --- at that stage we know we have released all locks and can exit cleanly. I have implemented this idea with the attached patch. It was already explained to you why this is a bad idea. regards, tom lane -- 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] Terminating a backend
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: When we get the termination signal, why can't we just set a global boolean, do a query cancel, and in the setjmp() code block check the global and exit --- at that stage we know we have released all locks and can exit cleanly. I have implemented this idea with the attached patch. It was already explained to you why this is a bad idea. Yes, I remember your comments: http://archives.postgresql.org/pgsql-hackers/2008-03/msg00145.php Keep in mind that 99% of the excuse for people to want to use SIGTERM is that the backend isn't responding to SIGINT. If you've fixed things so that SIGTERM cannot get them out of any situation that SIGINT doesn't get them out of, I don't think it's a step forward. ... [shrug...] They can do that now, most of the time. What this is about is dealing with corner cases, and in that respect what your proposal will do is replace soluble problems with insoluble ones. But I suppose I can't stop you if you're insistent. I need more than one person to tell me it is a bad idea because I think it is a good idea. If we tell people SIGTERM is not safe to use then why is making it safe worse. And if it is safe, we can just add a function to enable SIGTERM to the backend without doing the query cancel longjump(). -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] datum passed to macro which expects a pointer
This may seem a little pedantic but I noticed a few places where we pass a datum to a macro which treats the datum as a pointer. This works now but might not in the future (if, say, Datum were to be 8 bytes). Thanks, Gavin Index: src/backend/utils/adt/varlena.c === RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/adt/varlena.c,v retrieving revision 1.164 diff -c -p -c -r1.164 varlena.c *** src/backend/utils/adt/varlena.c 25 Mar 2008 22:42:44 - 1.164 --- src/backend/utils/adt/varlena.c 12 Apr 2008 21:10:01 - *** text_substring(Datum str, int32 start, i *** 754,760 * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(str) || VARATT_IS_EXTERNAL(str)) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); --- 754,761 * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(DatumGetPointer(str)) || ! VARATT_IS_EXTERNAL(DatumGetPointer(str))) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); Index: src/backend/utils/mb/mbutils.c === RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/mb/mbutils.c,v retrieving revision 1.69 diff -c -p -c -r1.69 mbutils.c *** src/backend/utils/mb/mbutils.c 9 Jan 2008 23:43:54 - 1.69 --- src/backend/utils/mb/mbutils.c 12 Apr 2008 21:16:09 - *** pg_convert_to(PG_FUNCTION_ARGS) *** 313,319 result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_BYTEA_P(result); } /* --- 313,319 result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_BYTEA_P(DatumGetPointer(result)); } /* *** pg_convert_from(PG_FUNCTION_ARGS) *** 340,346 * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_TEXT_P(result); } /* --- 340,346 * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_TEXT_P(DatumGetPointer(result)); } /* -- 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] datum passed to macro which expects a pointer
Gavin Sherry [EMAIL PROTECTED] writes: This may seem a little pedantic but I noticed a few places where we pass a datum to a macro which treats the datum as a pointer. This works now but might not in the future (if, say, Datum were to be 8 bytes). Yeah, definitely something to fix. I think though that the cases like this: ! PG_RETURN_TEXT_P(DatumGetPointer(result)); might as well just use PG_RETURN_DATUM instead of casting twice. Was this just eyeball inspection or did you find a compiler that would complain about this? regards, tom lane -- 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] datum passed to macro which expects a pointer
On Sat, Apr 12, 2008 at 06:02:39PM -0400, Tom Lane wrote: Gavin Sherry [EMAIL PROTECTED] writes: This may seem a little pedantic but I noticed a few places where we pass a datum to a macro which treats the datum as a pointer. This works now but might not in the future (if, say, Datum were to be 8 bytes). Yeah, definitely something to fix. I think though that the cases like this: ! PG_RETURN_TEXT_P(DatumGetPointer(result)); might as well just use PG_RETURN_DATUM instead of casting twice. Oh of course. Updated patch attached. Was this just eyeball inspection or did you find a compiler that would complain about this? I wish. It was actually thrown up when we (Greenplum) changed the macros to be inline functions as part of changing Datum to be 8 bytes. By using inline functions we get proper type checking from the compiler and since we have only a small number of target platforms and architectures, inlining isn't an issue. Thanks, Gavin Index: src/backend/utils/adt/varlena.c === RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/adt/varlena.c,v retrieving revision 1.164 diff -c -p -c -r1.164 varlena.c *** src/backend/utils/adt/varlena.c 25 Mar 2008 22:42:44 - 1.164 --- src/backend/utils/adt/varlena.c 12 Apr 2008 21:10:01 - *** text_substring(Datum str, int32 start, i *** 754,760 * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(str) || VARATT_IS_EXTERNAL(str)) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); --- 754,761 * If we're working with an untoasted source, no need to do an extra * copying step. */ ! if (VARATT_IS_COMPRESSED(DatumGetPointer(str)) || ! VARATT_IS_EXTERNAL(DatumGetPointer(str))) slice = DatumGetTextPSlice(str, slice_start, slice_size); else slice = (text *) DatumGetPointer(str); Index: src/backend/utils/mb/mbutils.c === RCS file: /Users/swm/pgsql-cvs/pgsql/src/backend/utils/mb/mbutils.c,v retrieving revision 1.69 diff -c -p -c -r1.69 mbutils.c *** src/backend/utils/mb/mbutils.c 9 Jan 2008 23:43:54 - 1.69 --- src/backend/utils/mb/mbutils.c 12 Apr 2008 22:55:49 - *** pg_convert_to(PG_FUNCTION_ARGS) *** 313,319 result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_BYTEA_P(result); } /* --- 313,319 result = DirectFunctionCall3(pg_convert, string, src_encoding_name, dest_encoding_name); ! PG_RETURN_DATUM(result); } /* *** pg_convert_from(PG_FUNCTION_ARGS) *** 340,346 * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_TEXT_P(result); } /* --- 340,346 * in this case it will be because we've told pg_convert to return one * that is valid as text in the current database encoding. */ ! PG_RETURN_DATUM(result); } /* -- 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] datum passed to macro which expects a pointer
Gavin Sherry [EMAIL PROTECTED] writes: I wish. It was actually thrown up when we (Greenplum) changed the macros to be inline functions as part of changing Datum to be 8 bytes. Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines. What is it you're trying to accomplish by making it wider on 32-bitters? regards, tom lane -- 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] datum passed to macro which expects a pointer
On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: Gavin Sherry [EMAIL PROTECTED] writes: I wish. It was actually thrown up when we (Greenplum) changed the macros to be inline functions as part of changing Datum to be 8 bytes. Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines. What is it you're trying to accomplish by making it wider on 32-bitters? I miss stated there. This was actually about making key 64 bit types pass by value instead of pass by reference. Thanks, Gavin -- 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] datum passed to macro which expects a pointer
Gavin Sherry [EMAIL PROTECTED] writes: might as well just use PG_RETURN_DATUM instead of casting twice. Oh of course. Updated patch attached. Applied, thanks. regards, tom lane -- 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] datum passed to macro which expects a pointer
Gavin Sherry [EMAIL PROTECTED] writes: On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: Gavin Sherry [EMAIL PROTECTED] writes: I wish. It was actually thrown up when we (Greenplum) changed the macros to be inline functions as part of changing Datum to be 8 bytes. Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines. What is it you're trying to accomplish by making it wider on 32-bitters? I miss stated there. This was actually about making key 64 bit types pass by value instead of pass by reference. There was a patch to do this posted recently here as well. http://archives.postgresql.org/pgsql-patches/2008-03/msg00335.php Hm. I suppose it's true that you could make Datum 64-bit even on 32-bit machines and make int8 and float8 pass-by-value. Seems unlikely to be a net win though. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning -- 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] datum passed to macro which expects a pointer
On Sun, Apr 13, 2008 at 01:42:02AM +0100, Gregory Stark wrote: Gavin Sherry [EMAIL PROTECTED] writes: On Sat, Apr 12, 2008 at 07:07:48PM -0400, Tom Lane wrote: Gavin Sherry [EMAIL PROTECTED] writes: I wish. It was actually thrown up when we (Greenplum) changed the macros to be inline functions as part of changing Datum to be 8 bytes. Hmmm ... Datum has been 8 bytes for many years, on 64-bit machines. What is it you're trying to accomplish by making it wider on 32-bitters? I miss stated there. This was actually about making key 64 bit types pass by value instead of pass by reference. There was a patch to do this posted recently here as well. http://archives.postgresql.org/pgsql-patches/2008-03/msg00335.php Hm. I suppose it's true that you could make Datum 64-bit even on 32-bit machines and make int8 and float8 pass-by-value. Seems unlikely to be a net win though. A very quick scan showed me that one bet is missed in this patch which we learned about the hard way: write_auth_file() assumes timestamptz is pass by reference. I'm also not sure if endianness is completely covered in the patch but it looks fairly accurate. I think PointerGetDatum() may need the union trick (it's late where I am). There were other places in the code which were assuming Datums were equivalent to pointers. I'll dig them up. Also, it means we can clean up parts of numeric.c which special case calls from aggregates. Seems like a pretty clean patch though. Thanks, Gavin -- 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] sh: Add support Renesas SuperH
I wrote: Nobuhiro Iwamatsu [EMAIL PROTECTED] writes: +#if defined(__sh__) /* Renesas SuperH */ Do they have any longer form of that macro? I looked into the gcc sources, and the answer seems to be no :-(. So we're stuck with __sh__. I'm still pretty skeptical about the adequacy of the asm parameters, though. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
