Thread: refactor CreateTupleDescCopy()

This patch refactors CreateTupleDescCopy() and
CreateTupleDescCopyConstr() to remove some duplicated code, and clean
things up a little bit.

-Neil

Neil Conway <neilc@samurai.com> writes:
> This patch refactors CreateTupleDescCopy() and
> CreateTupleDescCopyConstr() to remove some duplicated code, and clean
> things up a little bit.

I think this is taking the "avoid duplicated code" mantra a little far.
You've defined a subroutine that returns a TupleDesc that is internally
inconsistent and cannot usefully be used for anything until it's fixed
by the parent routines.  That seems to me to make the module more
complex and confusing rather than less so.  The amount of code saved is
IMHO not worth that price.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> I think this is taking the "avoid duplicated code" mantra a little
> far.  You've defined a subroutine that returns a TupleDesc that is
> internally inconsistent and cannot usefully be used for anything
> until it's fixed by the parent routines.

Fair enough. Another way to refactor this would be to implement both
CreateTupleDescCopy() and CreateTupleDescCopyConstr() in terms of an
internal function that takes a bool indicating whether or not to
include constraints in the returned TupleDesc. Now that I think about
it, we could also just change the API to remove
CreateTupleDescCopyConstr(), and replace it with an additional bool
parameter to CreateTupleDescCopy().

I'm leaning toward doing the latter, and updating the 30 odd call
sites this would affect. Does that sound good?

-Neil

Neil Conway <neilc@samurai.com> writes:
> Now that I think about
> it, we could also just change the API to remove
> CreateTupleDescCopyConstr(), and replace it with an additional bool
> parameter to CreateTupleDescCopy().

That would be okay with me.  It might be a good idea to change the name
completely (perhaps CopyTupleDesc() ?) as a means of catching places
that aren't correctly updated.  (I worry about add-on modules that might
not get recompiled between versions; they'd still link, but then crash,
if the routine name is the same.)

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> That would be okay with me.  It might be a good idea to change the
> name completely (perhaps CopyTupleDesc() ?) as a means of catching
> places that aren't correctly updated.

Done, and done -- a revised patch is attached.

-Neil

Your patch has been added to the PostgreSQL unapplied patches list at:

    http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

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


Neil Conway wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > That would be okay with me.  It might be a good idea to change the
> > name completely (perhaps CopyTupleDesc() ?) as a means of catching
> > places that aren't correctly updated.
>
> Done, and done -- a revised patch is attached.
>
> -Neil
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
>                http://www.postgresql.org/docs/faqs/FAQ.html

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

This patch was withdrawn by the author.

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

Neil Conway wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
> > That would be okay with me.  It might be a good idea to change the
> > name completely (perhaps CopyTupleDesc() ?) as a means of catching
> > places that aren't correctly updated.
>
> Done, and done -- a revised patch is attached.
>
> -Neil
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
>                http://www.postgresql.org/docs/faqs/FAQ.html

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073