Обсуждение: Re: Retroactively adding send and recv functions to a type?

On Tue, Aug 20, 2019 at 1:03 AM Johann 'Myrkraverk' Oskarsson
<johann@myrkraverk.com> wrote:
> The help for ALTER TABLE seems to indicate this is not supported directly.

I meant ALTER TYPE.  Adding the send and recv functions doesn't seem
to supported by ALTER TYPE.

Is there a workaround for this?

-- 
Johann

 I'm not from the internet, I just work there.

"Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> I meant ALTER TYPE.  Adding the send and recv functions doesn't seem
> to supported by ALTER TYPE.
> Is there a workaround for this?

You could manually update the pg_type row, and then if you were
being fussy, add pg_depend entries showing the type depends on
the functions.

            regards, tom lane

On Tue, Aug 20, 2019 at 1:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> > I meant ALTER TYPE.  Adding the send and recv functions doesn't seem
> > to supported by ALTER TYPE.
> > Is there a workaround for this?
>
> You could manually update the pg_type row, and then if you were
> being fussy, add pg_depend entries showing the type depends on
> the functions.

Can I do this in a future proof way?  That is, is there a way to make
that into an upgrade script, or will I make the extension
un-upgradable doing that?


-- 
Johann

 I'm not from the internet, I just work there.

"Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> On Tue, Aug 20, 2019 at 1:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> You could manually update the pg_type row, and then if you were
>> being fussy, add pg_depend entries showing the type depends on
>> the functions.

> Can I do this in a future proof way?  That is, is there a way to make
> that into an upgrade script, or will I make the extension
> un-upgradable doing that?

[ shrug... ]  Depends what you consider "future proof".  I should think
that if pg_type.typsend goes away or changes meaning, for example,
that would be reflective of changes large enough to break an extension
dabbling in binary I/O in other ways anyway.

Inserting new rows into pg_depend manually is a bit riskier, but I
don't think that catalog has changed since its inception, so it's
not all that risky.

In any case, you could limit the lifespan of the upgrade script,
if you roll it up into a new base install script ASAP.

            regards, tom lane

On 19/08/2019 19:32, Tom Lane wrote:
> "Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
>> I meant ALTER TYPE.  Adding the send and recv functions doesn't seem
>> to supported by ALTER TYPE.
>> Is there a workaround for this?
> You could manually update the pg_type row, and then if you were
> being fussy, add pg_depend entries showing the type depends on
> the functions.


We generally discourage updating the catalogs directly.  This was why I
wrote the CREATE/ALTER DATABASE patch in 2014 that you helped me with
(fbb1d7d73f8).


I'm willing to work on a patch for ALTER TYPE if it has a chance of
being accepted.

-- 

Vik Fearing

Greetings,

* Vik Fearing (vik.fearing@2ndquadrant.com) wrote:
> On 19/08/2019 19:32, Tom Lane wrote:
> > "Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> >> I meant ALTER TYPE.  Adding the send and recv functions doesn't seem
> >> to supported by ALTER TYPE.
> >> Is there a workaround for this?
> > You could manually update the pg_type row, and then if you were
> > being fussy, add pg_depend entries showing the type depends on
> > the functions.
>
> We generally discourage updating the catalogs directly.  This was why I
> wrote the CREATE/ALTER DATABASE patch in 2014 that you helped me with
> (fbb1d7d73f8).
>
> I'm willing to work on a patch for ALTER TYPE if it has a chance of
> being accepted.

Seems pretty clear that it'd be a useful thing to have, so +1 from me,
at least.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Vik Fearing (vik.fearing@2ndquadrant.com) wrote:
>> We generally discourage updating the catalogs directly.  This was why I
>> wrote the CREATE/ALTER DATABASE patch in 2014 that you helped me with
>> (fbb1d7d73f8).
>> I'm willing to work on a patch for ALTER TYPE if it has a chance of
>> being accepted.

> Seems pretty clear that it'd be a useful thing to have, so +1 from me,
> at least.

We'd have to be extremely circumspect about what aspects of a base
type could be altered after-the-fact.  Adding binary I/O functions
seems unproblematic, but I'm not very sure what else is.

            regards, tom lane

On Tue, Aug 20, 2019 at 2:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> "Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> > On Tue, Aug 20, 2019 at 1:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> You could manually update the pg_type row, and then if you were
> >> being fussy, add pg_depend entries showing the type depends on
> >> the functions.
>
> > Can I do this in a future proof way?  That is, is there a way to make
> > that into an upgrade script, or will I make the extension
> > un-upgradable doing that?
>
> [ shrug... ]  Depends what you consider "future proof".  I should think
> that if pg_type.typsend goes away or changes meaning, for example,
> that would be reflective of changes large enough to break an extension
> dabbling in binary I/O in other ways anyway.
>
> Inserting new rows into pg_depend manually is a bit riskier, but I
> don't think that catalog has changed since its inception, so it's
> not all that risky.

I have updated the catalog, and the binary send and recv functions work.

The steps I took are

  create function sha1_send( sha1 ) returns bytea immutable
  language c strict as 'hashtypes', 'sha_send1';

  update pg_type set typsend = 'sha1_send'::regproc
  where typname = 'sha1';

  create function sha1_recv( internal ) returns sha1 immutable
  language c strict as 'hashtypes', 'sha_recv1';

  update pg_type set typreceive = 'sha1_recv'::regproc
  where typname = 'sha1';

Then for completeness sake, I added two rows into pg_depend with

  insert into pg_depend ( classid, objid, objsubid, refclassid,
    refobjid, refobjsubid, deptype )
  values ( 'pg_type'::regclass::oid, 'sha1'::regtype::oid, 0,
    'pg_proc'::regclass::oid, 'sha1_recv'::regproc::oid, 0, 'n' );

  insert into pg_depend ( classid, objid, objsubid, refclassid,
    refobjid, refobjsubid, deptype )
  values ( 'pg_type'::regclass::oid, 'sha1'::regtype::oid, 0,
    'pg_proc'::regclass::oid, 'sha1_send'::regproc::oid, 0, 'n' );

Before I roll all of that into an upgrade script for the other sha
types, is there something else I should be doing?

I did not dare to try before adding to pg_depend, but here's what
happens when I try to drop function sha1_recv;

  ERROR:  cannot drop function sha1_recv(internal) because other
objects depend on it
  DETAIL:  extension hashtypes depends on function sha1_recv(internal)
  column passwd of table pwned depends on type sha1
  function sha1_send(sha1) depends on type sha1

Does this look correct?

> In any case, you could limit the lifespan of the upgrade script,
> if you roll it up into a new base install script ASAP.

I am not the maintainer of the extension, and I'll see what I can do.

-- 
Johann

 I'm not from the internet, I just work there.

"Johann 'Myrkraverk' Oskarsson" <johann@myrkraverk.com> writes:
> The steps I took are

>   create function sha1_send( sha1 ) returns bytea immutable
>   language c strict as 'hashtypes', 'sha_send1';

>   update pg_type set typsend = 'sha1_send'::regproc
>   where typname = 'sha1';

>   create function sha1_recv( internal ) returns sha1 immutable
>   language c strict as 'hashtypes', 'sha_recv1';

>   update pg_type set typreceive = 'sha1_recv'::regproc
>   where typname = 'sha1';

Those updates don't look very safe: for instance, what if there's
another type named sha1 in some other schema?  I'd do it like

-- create the functions
update pg_type set
  typsend = 'sha1_send(sha1)'::regprocedure,
  typreceive = 'sha1_recv(internal)'::regprocedure
where oid = 'sha1'::regtype;

This formulation only relies on your schema being frontmost in
the search path, which it should be during CREATE/ALTER EXTENSION.

> Then for completeness sake, I added two rows into pg_depend with

>   insert into pg_depend ( classid, objid, objsubid, refclassid,
>     refobjid, refobjsubid, deptype )
>   values ( 'pg_type'::regclass::oid, 'sha1'::regtype::oid, 0,
>     'pg_proc'::regclass::oid, 'sha1_recv'::regproc::oid, 0, 'n' );

>   insert into pg_depend ( classid, objid, objsubid, refclassid,
>     refobjid, refobjsubid, deptype )
>   values ( 'pg_type'::regclass::oid, 'sha1'::regtype::oid, 0,
>     'pg_proc'::regclass::oid, 'sha1_send'::regproc::oid, 0, 'n' );

You could skip the explicit casts to oid, and again I think use of
regprocedure would be safer than regproc.  Seems fine otherwise.

> I did not dare to try before adding to pg_depend, but here's what
> happens when I try to drop function sha1_recv;

>   ERROR:  cannot drop function sha1_recv(internal) because other
> objects depend on it
>   DETAIL:  extension hashtypes depends on function sha1_recv(internal)
>   column passwd of table pwned depends on type sha1
>   function sha1_send(sha1) depends on type sha1

> Does this look correct?

It looks a bit odd, but I think that just indicates that you created the
two functions manually rather than inside an extension update script,
so they're not known to be part of the extension.  You could experiment
with ALTER EXTENSION ADD to see if this output changes when they are
part of the extension.  (But you don't need ALTER EXTENSION ADD when
you create them in an update script.)

            regards, tom lane