Thread: pg_get_domaindef
Hi, Attached is a small patch that implements the pg_get_domaindef(oid) function. Somethings I am not sure about the patch and which I can fix based on your input are as follows. - I have used SPI interface. I saw that in ruleutils some methods make use of SPI interface whereas others use internal APIs. I wasn't sure about the reasons. If this is wrong, I can attempt a rewrite using internal APIs. - This function does not output the constraint name. I couldn't find the constraint name given by the user as part of the create domain command being stored in any system catalogs. The entry in pg_constraints store a system assigned name. But this may be because of my limited knowledge of the internals. - documentation. If this patch is good then I can do this later. There may be other issues, which I will be happy to fix. Rgds, Arul Shaji This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive thisemail in error, please return to sender. Thank you. If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please emailunsubscribe@fast.fujitsu.com.au
Attachment
On Sat, 2007-01-20 at 02:28 +1100, FAST PostgreSQL wrote:
> Attached is a small patch that implements the pg_get_domaindef(oid) function.
A few minor comments:
- don't use C++-style comments
- why does this code append a "-" to the output when SPI_processed != 1,
rather than erroring out?
+ if (spirc != SPI_OK_SELECT)
+ elog(ERROR, "failed to get pg_type tuple for domain %u",
domainOid);
+ if (SPI_processed != 1)
+ appendStringInfo(&buf, "-");
+ else
+ {
- you probably want to elog(ERROR) if typeTuple is invalid:
+ /*
+ * Get the base type of the domain
+ */
+ typeTuple = SearchSysCache(TYPEOID,
+ ObjectIdGetDatum(basetypeOid),
+ 0, 0, 0);
+
+ if (HeapTupleIsValid(typeTuple))
+ {
+ typebasetype = pstrdup(NameStr(((Form_pg_type)
GETSTRUCT(typeTuple))->typname));
+ appendStringInfo(&buf, "%s ",
quote_identifier(typebasetype));
+ }
+ ReleaseSysCache(typeTuple);
It's also debatable whether the function ought to be using a
*combination* of the syscaches and manual system catalog queries, since
these two views may be inconsistent. I guess this is a widespread
problem, though.
+ if (typnotnull || constraint != NULL)
+ {
+ if ( ( (contype != NULL) && (strcmp(contype,
"c") != 0) ) || typnotnull )
+ {
+ appendStringInfo(&buf, "CONSTRAINT ");
+ }
+ if (typnotnull)
+ {
+ appendStringInfo(&buf, "NOT NULL ");
+ }
+ }
+ if (constraint != NULL)
+ {
+ appendStringInfo(&buf,
quote_identifier(constraint));
+ }
This logic seems pretty awkward. Perhaps simpler would be a check for
typnotnull (and then appending "CONSTRAINT NOT NULL"), and then handling
the non-typnotnull branch separately.
-Neil
On Fri, 19 Jan 2007 17:02, Neil Conway wrote:
> On Sat, 2007-01-20 at 02:28 +1100, FAST PostgreSQL wrote:
> > Attached is a small patch that implements the pg_get_domaindef(oid)
> > function.
>
> A few minor comments:
>
> - don't use C++-style comments
OK. Can do.
>
> - why does this code append a "-" to the output when SPI_processed != 1,
> rather than erroring out?
get_ruledef() does the same. As the user gets a '-' in that case when a
non-existent oid is given, I just wanted to be consistent. Maybe a wrong
idea ?
>
> - you probably want to elog(ERROR) if typeTuple is invalid:
Of course.
> + if (typnotnull || constraint != NULL)
> + {
> + if ( ( (contype != NULL) && (strcmp(contype,
> "c") != 0) ) || typnotnull )
> + {
> + appendStringInfo(&buf, "CONSTRAINT ");
> + }
> + if (typnotnull)
> + {
> + appendStringInfo(&buf, "NOT NULL ");
> + }
> + }
> + if (constraint != NULL)
> + {
> + appendStringInfo(&buf,
> quote_identifier(constraint));
> + }
>
> This logic seems pretty awkward. Perhaps simpler would be a check for
> typnotnull (and then appending "CONSTRAINT NOT NULL"), and then handling
> the non-typnotnull branch separately.
Yeah agree.
>
> -Neil
Rgds,
Arul Shaji
This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the
ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged
information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive
thisemail in error, please return to sender. Thank you.
If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please
emailunsubscribe@fast.fujitsu.com.au
Please find attached the patch with modifications
Rgds,
Arul Shaji
On Sat, 20 Jan 2007 04:44, FAST PostgreSQL wrote:
> On Fri, 19 Jan 2007 17:02, Neil Conway wrote:
> > On Sat, 2007-01-20 at 02:28 +1100, FAST PostgreSQL wrote:
> > > Attached is a small patch that implements the pg_get_domaindef(oid)
> > > function.
> >
> > A few minor comments:
> >
> > - don't use C++-style comments
>
> OK. Can do.
>
> > - why does this code append a "-" to the output when SPI_processed != 1,
> > rather than erroring out?
>
> get_ruledef() does the same. As the user gets a '-' in that case when a
> non-existent oid is given, I just wanted to be consistent. Maybe a wrong
> idea ?
>
> > - you probably want to elog(ERROR) if typeTuple is invalid:
>
> Of course.
>
> > + if (typnotnull || constraint != NULL)
> > + {
> > + if ( ( (contype != NULL) && (strcmp(contype,
> > "c") != 0) ) || typnotnull )
> > + {
> > + appendStringInfo(&buf, "CONSTRAINT ");
> > + }
> > + if (typnotnull)
> > + {
> > + appendStringInfo(&buf, "NOT NULL ");
> > + }
> > + }
> > + if (constraint != NULL)
> > + {
> > + appendStringInfo(&buf,
> > quote_identifier(constraint));
> > + }
> >
> > This logic seems pretty awkward. Perhaps simpler would be a check for
> > typnotnull (and then appending "CONSTRAINT NOT NULL"), and then handling
> > the non-typnotnull branch separately.
>
> Yeah agree.
>
> > -Neil
>
> Rgds,
> Arul Shaji
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the
ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged
information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive
thisemail in error, please return to sender. Thank you.
If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please
emailunsubscribe@fast.fujitsu.com.au
Attachment
FAST PostgreSQL wrote: > Please find attached the patch with modifications > are you proposing to implement the other functions in this TODO item (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(), pg_get_tabledef(), pg_get_functiondef() ) ? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> FAST PostgreSQL wrote:
>> Please find attached the patch with modifications
> are you proposing to implement the other functions in this TODO item
> (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(),
> pg_get_tabledef(), pg_get_functiondef() ) ?
I haven't entirely understood the use case for any of these. It's not
pg_dump, for a number of reasons: one being that pg_dump still has to
support older backend versions, and another being that every time we
let backend SnapshotNow functions get involved, we take another hit to
pg_dump's claim to produce a consistent MVCC snapshot.
But my real objection is: do we really want to support duplicative code
in both pg_dump and the backend? Updating pg_dump is already a major
PITA whenever one adds a new feature; doubling that work isn't
attractive. (And it'd be double, not just a copy-and-paste, because of
the large difference in the operating environment.) So I want to hear a
seriously convincing use-case that will justify the maintenance load we
are setting up for ourselves. "Somebody might want this" is not
adequate.
Perhaps a better area of work would be the often-proposed refactoring of
pg_dump into a library and driver program, wherein the library could
expose individual functions such as "fetch the SQL definition of this
object". Unfortunately, that'll be a huge project with no payoff until
the end...
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > FAST PostgreSQL wrote: > >> Please find attached the patch with modifications > > > are you proposing to implement the other functions in this TODO item > > (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(), > > pg_get_tabledef(), pg_get_functiondef() ) ? > > I haven't entirely understood the use case for any of these. It's not > pg_dump, for a number of reasons: one being that pg_dump still has to > support older backend versions, and another being that every time we > let backend SnapshotNow functions get involved, we take another hit to > pg_dump's claim to produce a consistent MVCC snapshot. > > But my real objection is: do we really want to support duplicative code > in both pg_dump and the backend? Updating pg_dump is already a major > PITA whenever one adds a new feature; doubling that work isn't > attractive. (And it'd be double, not just a copy-and-paste, because of > the large difference in the operating environment.) So I want to hear a > seriously convincing use-case that will justify the maintenance load we > are setting up for ourselves. "Somebody might want this" is not > adequate. I realize it is problem to have the function in two places in our code, but if we don't make a user-accessible version, every application has to roll their own version and update it for our system catalog changes. -- Bruce Momjian bruce@momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> ... convincing use-case that will justify the maintenance load we
>> are setting up for ourselves. "Somebody might want this" is not
>> adequate.
> I realize it is problem to have the function in two places in our code,
> but if we don't make a user-accessible version, every application has to
> roll their own version and update it for our system catalog changes.
Nope, wrong, you are assuming the conclusion. Exactly which apps have
to have this?
regards, tom lane
On Thu, 25 Jan 2007, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> ... convincing use-case that will justify the maintenance load we > >> are setting up for ourselves. "Somebody might want this" is not > >> adequate. > > > I realize it is problem to have the function in two places in our code, > > but if we don't make a user-accessible version, every application has to > > roll their own version and update it for our system catalog changes. > > Nope, wrong, you are assuming the conclusion. Exactly which apps have > to have this? Well, the alternative interfaces like pgadmin and ppa. That said, I prefer the idea of breaking out the queries in pg_dump and psql into a library. Like you say up thread, that's a big project and it's an all or nothing proposition. Thanks, Gavin
On Wed, 24 Jan 2007, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > FAST PostgreSQL wrote: > >> Please find attached the patch with modifications > > > are you proposing to implement the other functions in this TODO item > > (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(), > > pg_get_tabledef(), pg_get_functiondef() ) ? > > I haven't entirely understood the use case for any of these. It's not > pg_dump, for a number of reasons: one being that pg_dump still has to > support older backend versions, and another being that every time we > let backend SnapshotNow functions get involved, we take another hit to > pg_dump's claim to produce a consistent MVCC snapshot. I was talking to AndrewSN on irc about this. He proposed that we supply two versions (yes I hear the collective groan) of the SQL functions: a fast one (SnapshotNow) and an accurate one (which doesn't use SnapshotNow). The accurate version is important not just for pg_dump but for a host of people who interact with the system catalogs. If anyone's wondering why people are interacting with system catalogs in the first place, they need look know further than a monitoring application which checks system health and sanity on a regular basis. Combine that with some of the SnapshotNow based get def functions and common enough DDL (like temp table creation) and you start getting errors which look much more serious than what they are. Implementing the accurate version might be done via SPI. This is a headache though. It's starting to look like pulling the guts out of pg_dump and putting it in a library :-). Maybe the read place for this is actually pgfoundry? Thanks, Gavin
Gavin Sherry <swm@alcove.com.au> writes:
> I was talking to AndrewSN on irc about this. He proposed that we supply
> two versions (yes I hear the collective groan) of the SQL functions: a
> fast one (SnapshotNow) and an accurate one (which doesn't use
> SnapshotNow).
Um, that's such a fundamental misconception that it's got to be nipped
in the bud. The reason the backend tends to operate on SnapshotNow is
that it can't afford to be working with obsolete schema data. As an
example, you'd certainly not be happy if your updates to a table
disappeared into nowhere because your backend was working against a
snapshot that said table X was in tablespace Y, when meanwhile someone
had committed a transaction that moved it to tablespace Z. On the other
hand, pg_dump is entirely not about applying updates; it would like to
have a consistent read-only snapshot as of a time that might be many
hours ago by the time it's done. Both viewpoints are "accurate" for
their respective purposes; neither is chosen because it is "fast".
We might indeed need two sets of functions, but if you categorize them
like that you'll never get it right.
regards, tom lane
On Thu, 25 Jan 2007, Tom Lane wrote: > Gavin Sherry <swm@alcove.com.au> writes: > > I was talking to AndrewSN on irc about this. He proposed that we supply > > two versions (yes I hear the collective groan) of the SQL functions: a > > fast one (SnapshotNow) and an accurate one (which doesn't use > > SnapshotNow). > > Um, that's such a fundamental misconception that it's got to be nipped > in the bud. The reason the backend tends to operate on SnapshotNow is Oops. Poor choice of words. Thanks, Gavin
On Thu, 25 Jan 2007 02:25, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > FAST PostgreSQL wrote: > >> Please find attached the patch with modifications > > > > are you proposing to implement the other functions in this TODO item > > (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(), > > pg_get_tabledef(), pg_get_functiondef() ) ? > > I haven't entirely understood the use case for any of these. It's not Any consensus on these functions? If we decide against having these then its better to remove them from the TODO list temporarily/permanently......... Rgds, Arul Shaji > pg_dump, for a number of reasons: one being that pg_dump still has to > support older backend versions, and another being that every time we > let backend SnapshotNow functions get involved, we take another hit to > pg_dump's claim to produce a consistent MVCC snapshot. > > But my real objection is: do we really want to support duplicative code > in both pg_dump and the backend? Updating pg_dump is already a major > PITA whenever one adds a new feature; doubling that work isn't > attractive. (And it'd be double, not just a copy-and-paste, because of > the large difference in the operating environment.) So I want to hear a > seriously convincing use-case that will justify the maintenance load we > are setting up for ourselves. "Somebody might want this" is not > adequate. > > Perhaps a better area of work would be the often-proposed refactoring of > pg_dump into a library and driver program, wherein the library could > expose individual functions such as "fetch the SQL definition of this > object". Unfortunately, that'll be a huge project with no payoff until > the end... > > regards, tom lane This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive thisemail in error, please return to sender. Thank you. If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please emailunsubscribe@fast.fujitsu.com.au
I have remove this TODO item:
* %Add pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(),
pg_get_tabledef(), pg_get_domaindef(), pg_get_functiondef()
These would be for application use, not for use by pg_dump.
Seems there is lack of interest in adding this feature because of
maintanance concerns.
The attached patch is rejected for the same reason. Sorry for the
confusion.
---------------------------------------------------------------------------
FAST PostgreSQL wrote:
> On Thu, 25 Jan 2007 02:25, Tom Lane wrote:
> > Andrew Dunstan <andrew@dunslane.net> writes:
> > > FAST PostgreSQL wrote:
> > >> Please find attached the patch with modifications
> > >
> > > are you proposing to implement the other functions in this TODO item
> > > (pg_get_acldef(), pg_get_typedefault(), pg_get_attrdef(),
> > > pg_get_tabledef(), pg_get_functiondef() ) ?
> >
> > I haven't entirely understood the use case for any of these. It's not
>
> Any consensus on these functions? If we decide against having these then its
> better to remove them from the TODO list temporarily/permanently.........
>
> Rgds,
> Arul Shaji
>
>
> > pg_dump, for a number of reasons: one being that pg_dump still has to
> > support older backend versions, and another being that every time we
> > let backend SnapshotNow functions get involved, we take another hit to
> > pg_dump's claim to produce a consistent MVCC snapshot.
> >
> > But my real objection is: do we really want to support duplicative code
> > in both pg_dump and the backend? Updating pg_dump is already a major
> > PITA whenever one adds a new feature; doubling that work isn't
> > attractive. (And it'd be double, not just a copy-and-paste, because of
> > the large difference in the operating environment.) So I want to hear a
> > seriously convincing use-case that will justify the maintenance load we
> > are setting up for ourselves. "Somebody might want this" is not
> > adequate.
> >
> > Perhaps a better area of work would be the often-proposed refactoring of
> > pg_dump into a library and driver program, wherein the library could
> > expose individual functions such as "fetch the SQL definition of this
> > object". Unfortunately, that'll be a huge project with no payoff until
> > the end...
> >
> > regards, tom lane
> This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the
ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged
information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive
thisemail in error, please return to sender. Thank you.
>
> If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please
emailunsubscribe@fast.fujitsu.com.au
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +