Postgres Pro
Downloads
Home   >   mailing lists

Thread: Re: [GENERAL] Notify argument?

Re: [GENERAL] Notify argument?

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The breakage will come when we lengthen NAMEDATALEN, which I plan to
> tackle for 7.3.  We will need to re-order the NOTIFY structure and put
> the NAMEDATALEN string at the end of the struct so differing namedatalen
> backend/clients will work.  If you want to break it, 7.3 would probably
> be the time to do it.  :-)  Users will need a recompile pre-7.3 to use
> notify for 7.3 and later anyway.

If we're going to change the structure anyway, let's fix it to be
independent of NAMEDATALEN.  Instead of
   char        relname[NAMEDATALEN];   int         be_pid;

let's do
   char       *relname;   int         be_pid;

This should require no source-level changes in calling C code, thanks
to C's equivalence between pointers and arrays.  We can preserve the
fact that freeing a PQnotifies result takes only one free() with a
little hacking to make the string be allocated in the same malloc call:
   newNotify = (PGnotify *) malloc(sizeof(PGnotify) + strlen(str) + 1);   newNotify->relname = (char *) newNotify +
sizeof(PGnotify);  strcpy(newNotify->relname, str);
 

Thus, with one line of extra ugliness inside the library, we solve the
problem permanently.
        regards, tom lane

Re: [GENERAL] Notify argument?

From
nconway@klamath.dyndns.org (Neil Conway)
Date:
On Wed, Mar 20, 2002 at 04:10:14PM -0500, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > The breakage will come when we lengthen NAMEDATALEN, which I plan to
> > tackle for 7.3.  We will need to re-order the NOTIFY structure and put
> > the NAMEDATALEN string at the end of the struct so differing namedatalen
> > backend/clients will work.  If you want to break it, 7.3 would probably
> > be the time to do it.  :-)  Users will need a recompile pre-7.3 to use
> > notify for 7.3 and later anyway.
> 
> If we're going to change the structure anyway, let's fix it to be
> independent of NAMEDATALEN.

Sounds good. If we're making other backwards-incompatible changes to
pgNotify, one thing that bugs me about the API is the use of "relname"
to refer to name of the NOTIFY/LISTEN condition. This is incorrect
because the name of a condition is _not_ the name of a relation -- there
is no necessary correspondence between these. The names of NOTIFY
conditions are arbitrary, and chosen by the application developer.

The same terminology is used in the backend NOTIFY/LISTEN code (e.g.
pg_listener), but the major source of incompatibility will be the change
to libpq. Would it be a good idea to rename "relname" to something more
sensible?

Cheers,

Neil

-- 
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Re: [GENERAL] Notify argument?

From
Bruce Momjian
Date:
Neil Conway wrote:
> On Wed, Mar 20, 2002 at 04:10:14PM -0500, Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > The breakage will come when we lengthen NAMEDATALEN, which I plan to
> > > tackle for 7.3.  We will need to re-order the NOTIFY structure and put
> > > the NAMEDATALEN string at the end of the struct so differing namedatalen
> > > backend/clients will work.  If you want to break it, 7.3 would probably
> > > be the time to do it.  :-)  Users will need a recompile pre-7.3 to use
> > > notify for 7.3 and later anyway.
> > 
> > If we're going to change the structure anyway, let's fix it to be
> > independent of NAMEDATALEN.
> 
> Sounds good. If we're making other backwards-incompatible changes to
> pgNotify, one thing that bugs me about the API is the use of "relname"
> to refer to name of the NOTIFY/LISTEN condition. This is incorrect
> because the name of a condition is _not_ the name of a relation -- there
> is no necessary correspondence between these. The names of NOTIFY
> conditions are arbitrary, and chosen by the application developer.
> 
> The same terminology is used in the backend NOTIFY/LISTEN code (e.g.
> pg_listener), but the major source of incompatibility will be the change
> to libpq. Would it be a good idea to rename "relname" to something more
> sensible?

Renaming the column would make an API change.  I was talking only about
requiring a recompile.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026

Re: [GENERAL] Notify argument?

From
Tom Lane
Date:
nconway@klamath.dyndns.org (Neil Conway) writes:
>> If we're going to change the structure anyway, let's fix it to be
>> independent of NAMEDATALEN.

> Sounds good. If we're making other backwards-incompatible changes to
> pgNotify, one thing that bugs me about the API is the use of "relname"
> to refer to name of the NOTIFY/LISTEN condition.

I hear you ... but my proposal only requires a recompile, *not* any
source code changes.  Renaming the field would break client source code.
I doubt it's worth that.
        regards, tom lane

Re: [GENERAL] Notify argument?

From
Neil Conway
Date:
On Thu, 2002-03-21 at 00:16, Tom Lane wrote:
> nconway@klamath.dyndns.org (Neil Conway) writes:
> >> If we're going to change the structure anyway, let's fix it to be
> >> independent of NAMEDATALEN.
> 
> > Sounds good. If we're making other backwards-incompatible changes to
> > pgNotify, one thing that bugs me about the API is the use of "relname"
> > to refer to name of the NOTIFY/LISTEN condition.
> 
> I hear you ... but my proposal only requires a recompile, *not* any
> source code changes.  Renaming the field would break client source code.
> I doubt it's worth that.

Okay, that's fair -- I'll leave it as it is.

Cheers,

Neil

-- 
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Re: [GENERAL] Notify argument?

From
Bruce Momjian
Date:
Here is a patch that implements Tom's suggestion of mallocing the
relation name string as part of PQnotify and not depending on
NAMEDATALEN.  Nice trick.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > The breakage will come when we lengthen NAMEDATALEN, which I plan to
> > tackle for 7.3.  We will need to re-order the NOTIFY structure and put
> > the NAMEDATALEN string at the end of the struct so differing namedatalen
> > backend/clients will work.  If you want to break it, 7.3 would probably
> > be the time to do it.  :-)  Users will need a recompile pre-7.3 to use
> > notify for 7.3 and later anyway.
>
> If we're going to change the structure anyway, let's fix it to be
> independent of NAMEDATALEN.  Instead of
>
>     char        relname[NAMEDATALEN];
>     int         be_pid;
>
> let's do
>
>     char       *relname;
>     int         be_pid;
>
> This should require no source-level changes in calling C code, thanks
> to C's equivalence between pointers and arrays.  We can preserve the
> fact that freeing a PQnotifies result takes only one free() with a
> little hacking to make the string be allocated in the same malloc call:
>
>     newNotify = (PGnotify *) malloc(sizeof(PGnotify) + strlen(str) + 1);
>     newNotify->relname = (char *) newNotify + sizeof(PGnotify);
>     strcpy(newNotify->relname, str);
>
> Thus, with one line of extra ugliness inside the library, we solve the
> problem permanently.
>
>             regards, tom lane
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
Index: src/interfaces/libpq/fe-exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.118
diff -c -r1.118 fe-exec.c
*** src/interfaces/libpq/fe-exec.c    8 Apr 2002 03:48:10 -0000    1.118
--- src/interfaces/libpq/fe-exec.c    15 Apr 2002 00:15:29 -0000
***************
*** 1510,1517 ****
          return EOF;
      if (pqGets(&conn->workBuffer, conn))
          return EOF;
!     newNotify = (PGnotify *) malloc(sizeof(PGnotify));
!     strncpy(newNotify->relname, conn->workBuffer.data, NAMEDATALEN);
      newNotify->be_pid = be_pid;
      DLAddTail(conn->notifyList, DLNewElem(newNotify));
      return 0;
--- 1510,1525 ----
          return EOF;
      if (pqGets(&conn->workBuffer, conn))
          return EOF;
!
!     /*
!      * Store the relation name right after the PQnotify structure so it can
!      * all be freed at once.  We don't use NAMEDATALEN because we don't
!      * want to tie this interface to a specific server name length.
!      */
!     newNotify = (PGnotify *) malloc(sizeof(PGnotify) +
!                 strlen(conn->workBuffer.data) + 1);
!     newNotify->relname = (char *)newNotify + sizeof(PGnotify);
!     strcpy(newNotify->relname, conn->workBuffer.data);
      newNotify->be_pid = be_pid;
      DLAddTail(conn->notifyList, DLNewElem(newNotify));
      return 0;
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.83
diff -c -r1.83 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h    5 Mar 2002 06:07:26 -0000    1.83
--- src/interfaces/libpq/libpq-fe.h    15 Apr 2002 00:15:40 -0000
***************
*** 105,112 ****
   */
  typedef struct pgNotify
  {
!     char        relname[NAMEDATALEN];    /* name of relation containing
!                                          * data */
      int            be_pid;            /* process id of backend */
  } PGnotify;

--- 105,111 ----
   */
  typedef struct pgNotify
  {
!     char        *relname;        /* name of relation containing data */
      int            be_pid;            /* process id of backend */
  } PGnotify;

Re: [GENERAL] Notify argument?

From
Bruce Momjian
Date:
Fix applied.

---------------------------------------------------------------------------

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > The breakage will come when we lengthen NAMEDATALEN, which I plan to
> > tackle for 7.3.  We will need to re-order the NOTIFY structure and put
> > the NAMEDATALEN string at the end of the struct so differing namedatalen
> > backend/clients will work.  If you want to break it, 7.3 would probably
> > be the time to do it.  :-)  Users will need a recompile pre-7.3 to use
> > notify for 7.3 and later anyway.
> 
> If we're going to change the structure anyway, let's fix it to be
> independent of NAMEDATALEN.  Instead of
> 
>     char        relname[NAMEDATALEN];
>     int         be_pid;
> 
> let's do
> 
>     char       *relname;
>     int         be_pid;
> 
> This should require no source-level changes in calling C code, thanks
> to C's equivalence between pointers and arrays.  We can preserve the
> fact that freeing a PQnotifies result takes only one free() with a
> little hacking to make the string be allocated in the same malloc call:
> 
>     newNotify = (PGnotify *) malloc(sizeof(PGnotify) + strlen(str) + 1);
>     newNotify->relname = (char *) newNotify + sizeof(PGnotify);
>     strcpy(newNotify->relname, str);
> 
> Thus, with one line of extra ugliness inside the library, we solve the
> problem permanently.
> 
>             regards, tom lane
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026