Re: [PATCHES] [HACKERS] Text <-> C string
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > One of the questions in the original patch submission was whether it > would be worth changing all those DirectFunctionCall(textin) and > (textout) calls to use the new functions. Is it worthwhile avoiding > the fmgr overhead? I think that's worth doing just on notational clarity grounds. The small cycle savings doesn't excite me, but understanding DirectFunctionCall1(textin, CStringGetDatum(foo)) just involves more different bits of trivia than cstring_to_text(foo). > Last time I looked, the codebase had shifted quite a bit since I > originally wrote the patch. So it probably needs some work to apply > cleanly on the latest sources anyway. Yeah, with wide-impact patches like this you are always going to have that problem. One point though is that we don't have to improve every call site at the same time. I'd be inclined to put in the new functions and hit some representative sample of utils/adt/ files in the first commit, and then incrementally fix other stuff. 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] ALTER TYPE RENAME
This has been applied by Tom.
---
Petr Jelinek wrote:
> Tom Lane wrote:
> > Hm, I'm not entirely sure if you got the point or not. For either
> > relations or composite types, there is both a pg_class entry and a
> > pg_type entry, and their names *must* stay in sync. We could allow
> > people to rename both entries using either ALTER TABLE or ALTER TYPE,
> > but the general consensus seems to be that ALTER TYPE should be used
> > for composite types and ALTER TABLE for tables/views/etc. The fact
> > that there's a pg_class entry for a composite type is really an
> > implementation detail that would best not be exposed to users, so
> > enforcing the use of the appropriate command seems reasonable to me.
> >
> > regards, tom lane
> >
> Yes, that's exactly what I meant (my language skills are not best).
>
> Anyway, I am sending second version of the patch. Changes are:
> - renamed TypeRename function to RenameTypeInternal and changed its
> header comment
> - throw error when using ALTER TYPE to rename rowtype
> - split function renamerel to RenameRelation and RenameRelationInternal
> where RenameRelation does permission checks and stuff and also checks if
> it's not used for composite types and RenameRelationInternal does the
> actual rename. And I also did a little cleanup in those functions
> (removed unused code and changed some hardcoded relkind types to globaly
> predefined constants)
> - RenameType now calls RenameRelationInternal for composite types
> (which calls RenameTypeInternal automatically)
>
> Any other comments ?
>
> --
> Regards
> Petr Jelinek (PJMODOS)
>
> Index: doc/src/sgml/ref/alter_type.sgml
> ===
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/alter_type.sgml,v
> retrieving revision 1.4
> diff -c -r1.4 alter_type.sgml
> *** doc/src/sgml/ref/alter_type.sgml 16 Sep 2006 00:30:16 - 1.4
> --- doc/src/sgml/ref/alter_type.sgml 29 Sep 2007 05:43:14 -
> ***
> *** 26,31
> --- 26,32
>
> ALTER TYPE name OWNER TO
> new_owner
> ALTER TYPE name SET SCHEMA
> new_schema
> + ALTER TYPE name RNAME TO
> new_name
>
>
>
> ***
> *** 83,88
> --- 84,98
>
>
>
> +
> + new_name
> +
> +
> + The new name for the type.
> +
> +
> +
> +
>
>
>
> Index: src/backend/catalog/pg_type.c
> ===
> RCS file: /projects/cvsroot/pgsql/src/backend/catalog/pg_type.c,v
> retrieving revision 1.113
> diff -c -r1.113 pg_type.c
> *** src/backend/catalog/pg_type.c 12 May 2007 00:54:59 - 1.113
> --- src/backend/catalog/pg_type.c 30 Sep 2007 04:20:03 -
> ***
> *** 552,566
> }
>
> /*
> ! * TypeRename
>* This renames a type, as well as any associated array type.
>*
> ! * Note: this isn't intended to be a user-exposed function; it doesn't check
> ! * permissions etc. (Perhaps TypeRenameInternal would be a better name.)
> ! * Currently this is only used for renaming table rowtypes.
>*/
> void
> ! TypeRename(Oid typeOid, const char *newTypeName, Oid typeNamespace)
> {
> Relationpg_type_desc;
> HeapTuple tuple;
> --- 552,567
> }
>
> /*
> ! * RenameTypeInternal
>* This renames a type, as well as any associated array type.
>*
> ! * Caller must have already checked privileges.
> ! *
> ! * Currently this is used for renaming table rowtypes and for
> ! * ALTER TYPE RENAME TO command.
>*/
> void
> ! RenameTypeInternal(Oid typeOid, const char *newTypeName, Oid typeNamespace)
> {
> Relationpg_type_desc;
> HeapTuple tuple;
> ***
> *** 606,612
> {
> char *arrname = makeArrayTypeName(newTypeName, typeNamespace);
>
> ! TypeRename(arrayOid, arrname, typeNamespace);
> pfree(arrname);
> }
> }
> --- 607,613
> {
> char *arrname = makeArrayTypeName(newTypeName, typeNamespace);
>
> ! RenameTypeInternal(arrayOid, arrname, typeNamespace);
> pfree(arrname);
> }
> }
> ***
> *** 706,712
> newname = makeArrayTypeName(typeName, typeNamespace);
>
> /* Apply the rename */
> ! TypeRename(typeOid, newname, typeNamespace);
>
> /*
>* We must bump the command counter so that any subsequent use of
> --- 707,713
> newname = makeArrayTypeName(typeName, typeNamespace);
>
> /* Apply the rename */
> ! RenameTypeInternal(typeOid, newname, typeNamespace);
>
> /*
>* We must bump the command counter so that any subsequent use of
> Index: src/backend/commands/alter.c
>
Re: [PATCHES] [HACKERS] Text <-> C string
On 20/03/2008, Tom Lane <[EMAIL PROTECTED]> wrote: > > I started to look at applying this patch and immediately decided that > I didn't like these names --- it's exceeding un-obvious which direction > of conversion any one of the functions performs. Seems like every time > you wanted to call one, you'd be going back to look at the source code > to remember which to use. > That's a fair criticism. I wanted to make the function names as compact as possible, but your comment about the directionality of the functions rings true. > What do people think of text_to_cstring? Or should we go with > TextPGetCString for consistency with the Datum-whacking macros? (I seem > to recall having argued against the latter idea, but am reconsidering.) > Or something else? > Your original argument against FooGetBar was that it would be *too* consistent with the Datum macros, leading people to think that these functions actually were macros. As long as we don't want people getting confused about the function/macro distinction, that argument still makes sense to me, and I'd be more inclined towards the foo_to_bar() convention. > BTW, I suspect that the _limit functions are mostly useless --- > a quick look through the patch did not suggest that any of the places > using them really needed a limit. The point of, for instance, > TZ_STRLEN_MAX is to be able to use fixed-size local buffers, and > if you're going to pay for a palloc anyway then you might as well > forget it. What about callers like dotrim() in oracle_compat.c, which only want to copy characters from the source string up to a particular length? Doesn't that indicate a legitimate requirement for a cstring_to_text_limit() (the call site was palloc'ing the text value anyway)? On the other hand, we do have those call sites (TZ_STRLEN_MAX is a good example) where the caller just wanted to use a local buffer. In which case your strlcpy-equivalent function would probably be the right thing to offer. > (There might be some value in a strlcpy equivalent that > copies from a text datum into a limited-size caller-supplied buffer, > but that's not what we've got here.) > Perhaps we keep cstring_to_text_limit(), but make text_to_cstring_limit() behave like strlcpy() instead? One of the questions in the original patch submission was whether it would be worth changing all those DirectFunctionCall(textin) and (textout) calls to use the new functions. Is it worthwhile avoiding the fmgr overhead? Thanks for taking the time to review my patch and provide this feedback. I'm happy to send in an updated version once we settle on the naming convention for the functions. Last time I looked, the codebase had shifted quite a bit since I originally wrote the patch. So it probably needs some work to apply cleanly on the latest sources anyway. Regards, 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] ALTER TYPE RENAME
Petr Jelinek <[EMAIL PROTECTED]> writes: > Anyway, I am sending second version of the patch. Applied with minor corrections. 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] Text <-> C string
"Brendan Jurd" <[EMAIL PROTECTED]> writes: > As discussed on -hackers, I'm trying to get rid of some redundant code > by creating a widely useful set of functions to convert between text > and C string in the backend. > The new extern functions, declared in include/utils/builtins.h and > defined in backend/utils/adt/varlena.c, are: > char * text_cstring(const text *t) > char * text_cstring_limit(const text *t, int len) > text * cstring_text(const char *s) > text * cstring_text_limit(const char *s, int len) I started to look at applying this patch and immediately decided that I didn't like these names --- it's exceeding un-obvious which direction of conversion any one of the functions performs. Seems like every time you wanted to call one, you'd be going back to look at the source code to remember which to use. What do people think of text_to_cstring? Or should we go with TextPGetCString for consistency with the Datum-whacking macros? (I seem to recall having argued against the latter idea, but am reconsidering.) Or something else? BTW, I suspect that the _limit functions are mostly useless --- a quick look through the patch did not suggest that any of the places using them really needed a limit. The point of, for instance, TZ_STRLEN_MAX is to be able to use fixed-size local buffers, and if you're going to pay for a palloc anyway then you might as well forget it. (There might be some value in a strlcpy equivalent that copies from a text datum into a limited-size caller-supplied buffer, but that's not what we've got here.) 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: [HACKERS] [PATCHES] Fix for large file support (nonsegment mode support)
Peter Eisentraut napsal(a): Zdenek Kotala wrote: But how it was mentioned in this thread maybe somethink like this "CREATE TABLESPACE name LOCATION '/my/location' SEGMENTS 10GB" should good solution. If segments is not mentioned then default value is used. I think you would need a tool to resegmentize a table or tablespace offline, usable for example when recovering a backup. Do you mean something like strip(1) command? I don't see any usecase for terrabytes data. You usually have a problem to find place where you can backup. Also, tablespace configuration information is of course also stored in a table. pg_tablespace probably won't become large, but it would probably still need to be special-cased, along with other system catalogs perhaps. It is true and unfortunately singularity. Same as database list which is in a table as well, but it is stored also as a text file for startup purpose. I more incline to use non table configuration file for tablespaces, because I don't see any advantage to have it under MVCC control and it allow also to define storage for pg_global and pg_default. An then, how to coordindate offline resegmenting and online tablespace operations in a crash-safe way? Another factor I just thought of is that tar, commonly used as part of a backup procedure, can on some systems only handle files up to 8 GB in size. There are supposed to be newer formats that can avoid that restriction, but it's not clear how widely available these are and what the incantation is to get at them. Of course we don't use tar directly, but if we ever make large segments the default, we ought to provide some clear advice for the user on how to make their backups. I think tar is OK - minimal on Solaris. See man largefile. Default segment size still should be 1GB. If DBA makes a decision to increase this to higher value, then it is his responsibility to find way how to process this big files. Zdenek -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [HACKERS] [PATCHES] Fix for large file support (nonsegment mode support)
On Wed, Mar 19, 2008 at 10:51:12AM +0100, Martijn van Oosterhout wrote: > On Wed, Mar 19, 2008 at 09:38:12AM +0100, Peter Eisentraut wrote: > > Another factor I just thought of is that tar, commonly used as part of a > > backup procedure, can on some systems only handle files up to 8 GB in size. > > > > There are supposed to be newer formats that can avoid that restriction, but > > it's not clear how widely available these are and what the incantation is > > to > > get at them. Of course we don't use tar directly, but if we ever make > > large > > segments the default, we ought to provide some clear advice for the user on > > how to make their backups. > > By my reading, GNU tar handles larger files and no-one else (not even a > POSIX standard tar) can... > > Have a nice day, > -- > Martijn van Oosterhout <[EMAIL PROTECTED]> http://svana.org/kleptog/ > > Please line up in a tree and maintain the heap invariant while > > boarding. Thank you for flying nlogn airlines. The star program written by Joerg Schilling is a very well written POSIX compatible tar program that can easily handle files larger than 8GB. It is another backup option. Cheers, Ken -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [HACKERS] [PATCHES] Fix for large file support (nonsegment mode support)
On Wed, Mar 19, 2008 at 09:38:12AM +0100, Peter Eisentraut wrote: > Another factor I just thought of is that tar, commonly used as part of a > backup procedure, can on some systems only handle files up to 8 GB in size. > There are supposed to be newer formats that can avoid that restriction, but > it's not clear how widely available these are and what the incantation is to > get at them. Of course we don't use tar directly, but if we ever make large > segments the default, we ought to provide some clear advice for the user on > how to make their backups. By my reading, GNU tar handles larger files and no-one else (not even a POSIX standard tar) can... Have a nice day, -- Martijn van Oosterhout <[EMAIL PROTECTED]> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines. signature.asc Description: Digital signature
Re: [HACKERS] [PATCHES] Fix for large file support (nonsegment mode support)
Zdenek Kotala wrote: > But how it was mentioned in this thread maybe > somethink like this "CREATE TABLESPACE name LOCATION '/my/location' > SEGMENTS 10GB" should good solution. If segments is not mentioned then > default value is used. I think you would need a tool to resegmentize a table or tablespace offline, usable for example when recovering a backup. Also, tablespace configuration information is of course also stored in a table. pg_tablespace probably won't become large, but it would probably still need to be special-cased, along with other system catalogs perhaps. An then, how to coordindate offline resegmenting and online tablespace operations in a crash-safe way? Another factor I just thought of is that tar, commonly used as part of a backup procedure, can on some systems only handle files up to 8 GB in size. There are supposed to be newer formats that can avoid that restriction, but it's not clear how widely available these are and what the incantation is to get at them. Of course we don't use tar directly, but if we ever make large segments the default, we ought to provide some clear advice for the user on how to make their backups. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
