Text <-> C string

"Brendan Jurd" <direvus@gmail.com> writes:

Surely having the exact same four lines of code written out in dozens
of places is a Bad Thing, but perhaps there is some reasoning behind
this that I am missing?

The canonical way to do it is with

DatumGetCString(DirectFunctionCall1(textout, t))

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

On 9/22/07, Gregory Stark <stark@enterprisedb.com> wrote:

The canonical way to do it is with

DatumGetCString(DirectFunctionCall1(textout, t))

Ah, I see. Thanks.

In that case, would it be helpful if I submitted a patch for the
various code fragments that do this locally, updating them to use
DatumGetCString?

"Brendan Jurd" <direvus@gmail.com> writes:

On 9/22/07, Gregory Stark <stark@enterprisedb.com> wrote:

The canonical way to do it is with

DatumGetCString(DirectFunctionCall1(textout, t))

Ah, I see. Thanks.

In that case, would it be helpful if I submitted a patch for the
various code fragments that do this locally, updating them to use
DatumGetCString?

I would be interested in seeing just a list of such places if you have it
handy. I don't think we consider it wrong to violate the text data type
abstraction barrier like you describe though.

I'm interested because any such code is possibly either failing to take into
account toasted data or is unnecessarily detoasting packed varlenas.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

On 9/22/07, Gregory Stark <stark@enterprisedb.com> wrote:

The canonical way to do it is with

DatumGetCString(DirectFunctionCall1(textout, t))

I just noticed a couple of macros defined in src/include/tsearch/ts_utils.h:

#define TextPGetCString(t)
DatumGetCString(DirectFunctionCall1(textout, PointerGetDatum(t)))
#define CStringGetTextP(c) DatumGetTextP(DirectFunctionCall1(textin,
CStringGetDatum(c)))

Seems these would actually be convenient in quite a lot of places in
the backend. Is there any downside to moving these two into
src/include/postgres.h?

Well, a couple of specific cases that I came across are
quote_identifier() in src/backend/utils/adt/quote.c, and
do_to_timestamp() in src/backend/utils/adt/formatting.c (line 3349).

I was getting a rough notion of how common the duplication was using

$ egrep -Rn -C 2 'memcpy.*VARDATA' src/backend

Not all of these are genuine duplications of textout and textin (you
have to eyeball them individually to work that out) but it's a
reasonable starting point.

The files matched under src/backend are as follows.

src/backend/libpq/be-fsstubs.c
src/backend/utils/mb/mbutils.c
src/backend/utils/adt/timestamp.c
src/backend/utils/adt/nabstime.c
src/backend/utils/adt/xml.c
src/backend/utils/adt/quote.c
src/backend/utils/adt/oracle_compat.c
src/backend/utils/adt/varchar.c
src/backend/utils/adt/ruleutils.c
src/backend/utils/adt/varlena.c
src/backend/utils/adt/tsginidx.c
src/backend/utils/adt/cash.c
src/backend/utils/adt/date.c
src/backend/utils/adt/genfile.c
src/backend/utils/adt/network.c
src/backend/utils/adt/selfuncs.c
src/backend/utils/adt/formatting.c
src/backend/utils/adt/version.c
src/backend/utils/adt/pgstatfuncs.c
src/backend/access/heap/tuptoaster.c
src/backend/access/common/heaptuple.c
src/backend/storage/large_object/inv_api.c
src/backend/executor/execQual.c
src/backend/catalog/pg_conversion.c

"Brendan Jurd" <direvus@gmail.com> writes:

I just noticed a couple of macros defined in src/include/tsearch/ts_utils.h:

#define TextPGetCString(t)
DatumGetCString(DirectFunctionCall1(textout, PointerGetDatum(t)))
#define CStringGetTextP(c) DatumGetTextP(DirectFunctionCall1(textin,
CStringGetDatum(c)))

Seems these would actually be convenient in quite a lot of places in
the backend. Is there any downside to moving these two into
src/include/postgres.h?

I think if you look around you'll find several similar things in various
contrib modules. It would make some sense to try to unify all this.
I'm not particularly for making it macros in postgres.h though ---
that's no help if the macros require referencing stuff in builtins.h.

On grounds of code-space savings I think it might be worth making
these things be simple functions declared in builtins.h; that would
also make it much easier to change their implementations.

regards, tom lane

On 9/22/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Brendan Jurd" <direvus@gmail.com> writes:

I just noticed a couple of macros defined in src/include/tsearch/ts_utils.h:

#define TextPGetCString(t)
DatumGetCString(DirectFunctionCall1(textout, PointerGetDatum(t)))
#define CStringGetTextP(c) DatumGetTextP(DirectFunctionCall1(textin,
CStringGetDatum(c)))

I think if you look around you'll find several similar things in various
contrib modules. It would make some sense to try to unify all this.
I'm not particularly for making it macros in postgres.h though ---
that's no help if the macros require referencing stuff in builtins.h.

On grounds of code-space savings I think it might be worth making
these things be simple functions declared in builtins.h; that would
also make it much easier to change their implementations.

You're right about finding similar things in various places. Even
varlena.c has a set of these macros (PG_TEXT_GET_STR etc), but it
doesn't look they've really been utilised.

I'm happy to take a swing at this. Declaring in builtins.h makes sense.

The thing that's got me confused at the moment is what naming
convention to use for the functions. Looking in builtins.h you might
get the impression that we use lower_underscore for functions that are
called via fmgr, UPPER_UNDERSCORE for macros and CamelCase for
ordinary internal C functions, but there are plenty of exceptions to
disprove that rule. I see camel cased macros and lowercased internal
functions. Camel cased identifiers sometimes start with uppercase,
sometimes lowercase.

So the name for the text -> cstring function could be any of:

text_cstr
text_to_cstr
textToCString
TextToCString

Is there any kind of authoritative naming convention I can refer to?

Thanks for your time,
BJ

"Brendan Jurd" <direvus@gmail.com> writes:

The thing that's got me confused at the moment is what naming
convention to use for the functions.

Well, almost any convention you like has some precedent somewhere in
the PG code, given all the contributors over the years. Almost the
only thing we actively discourage is Hungarian notation, and I think
there's even some of that in some corners.

Personally I would vote against something like TextPGetCString because
it would look like one of the family of macros that are named FooGetBar.
Maybe use text_to_cstring and cstring_to_text? It's not real important
though.

regards, tom lane

On 9/22/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

On grounds of code-space savings I think it might be worth making
these things be simple functions declared in builtins.h; that would
also make it much easier to change their implementations.

I've noticed that this pattern isn't exclusive to the text type; other
varlena types like bytea and xmltype seem to have a common requirement
to translate to and fro C strings for various jobs.

Does it make sense to go one level lower, and make these functions
work for any varlena?

So far, I've got the following functions doing the work:

char * text_cstring(text *t)
char * text_cstring_limit(text *t, int len)
text * cstring_text(char *s)

It wouldn't be difficult at this point to make those functions
'varlena' rather than 'text', and then bytea and xmltype (and any
other future types that want to inherit from varlena) can take
advantage of them.

Thanks for your time,
BJ

"Brendan Jurd" <direvus@gmail.com> writes:

So far, I've got the following functions doing the work:

char * text_cstring(text *t)
char * text_cstring_limit(text *t, int len)
text * cstring_text(char *s)

It wouldn't be difficult at this point to make those functions
'varlena' rather than 'text', and then bytea and xmltype (and any
other future types that want to inherit from varlena) can take
advantage of them.

Mmm, but the conversions are generally not identical --- for instance,
bytea needs to do escaping/de-escaping, and I doubt that XML will stick
to dumb flat-string representation for long, and for that matter text
itself is likely to change someday for better locale support. Where the
representations and conversions *are* identical, one can just cast.
I'd vote for keeping the names focused on text ...

regards, tom lane

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)

Within varlena.c, the actual conversions are performed by:

char * do_text_cstring(const text *t, const int len)
text * do_cstring_text(const char *s, const int len)

These functions now do the work for the fmgr functions textin and
textout, as well as being directly accessible by backend code.

I've searched through the backend for any code which converted between
text and C string manually (with memcpy and VARDATA), replacing with
calls to one of the four new functions as appropriate.

I came across some areas which were using the same, or similar,
conversion technique on other varlena data types, such as bytea or
xmltype. In cases where the conversion was completely identical I
used the new functions. In cases with any differences (even if they
seemed minor) I played it safe and left them alone.

I'd now like to submit my work so far for review. This patch compiled
cleanly on Linux and passed all parallel regression tests. It appears
to be performance-neutral based on a few rough tests; I haven't tried
to profile the changes in detail.

There is still a lot of code out there using DirectFunctionCall1 to
call text(in|out)). I've decided to wait for some community feedback
on the patch as it stands before replacing those calls. There are a
great many, and it would be a shame to have to go through them more
than once.

I would naively expect that replacing fmgr calls with direct calls
would lead to a performance gain (no fmgr overhead), but honestly I'm
not sure whether that would actually make a difference.

Thanks for your time,
BJ

This has been saved for the 8.4 release:

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

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

Brendan Jurd wrote:

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)

Within varlena.c, the actual conversions are performed by:

char * do_text_cstring(const text *t, const int len)
text * do_cstring_text(const char *s, const int len)

These functions now do the work for the fmgr functions textin and
textout, as well as being directly accessible by backend code.

I've searched through the backend for any code which converted between
text and C string manually (with memcpy and VARDATA), replacing with
calls to one of the four new functions as appropriate.

I came across some areas which were using the same, or similar,
conversion technique on other varlena data types, such as bytea or
xmltype. In cases where the conversion was completely identical I
used the new functions. In cases with any differences (even if they
seemed minor) I played it safe and left them alone.

I'd now like to submit my work so far for review. This patch compiled
cleanly on Linux and passed all parallel regression tests. It appears
to be performance-neutral based on a few rough tests; I haven't tried
to profile the changes in detail.

There is still a lot of code out there using DirectFunctionCall1 to
call text(in|out)). I've decided to wait for some community feedback
on the patch as it stands before replacing those calls. There are a
great many, and it would be a shame to have to go through them more
than once.

I would naively expect that replacing fmgr calls with direct calls
would lead to a performance gain (no fmgr overhead), but honestly I'm
not sure whether that would actually make a difference.

Thanks for your time,
BJ

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

This has been saved for the 8.4 release:

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

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

Brendan Jurd wrote:

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)

Within varlena.c, the actual conversions are performed by:

char * do_text_cstring(const text *t, const int len)
text * do_cstring_text(const char *s, const int len)

These functions now do the work for the fmgr functions textin and
textout, as well as being directly accessible by backend code.

I've searched through the backend for any code which converted between
text and C string manually (with memcpy and VARDATA), replacing with
calls to one of the four new functions as appropriate.

I came across some areas which were using the same, or similar,
conversion technique on other varlena data types, such as bytea or
xmltype. In cases where the conversion was completely identical I
used the new functions. In cases with any differences (even if they
seemed minor) I played it safe and left them alone.

I'd now like to submit my work so far for review. This patch compiled
cleanly on Linux and passed all parallel regression tests. It appears
to be performance-neutral based on a few rough tests; I haven't tried
to profile the changes in detail.

There is still a lot of code out there using DirectFunctionCall1 to
call text(in|out)). I've decided to wait for some community feedback
on the patch as it stands before replacing those calls. There are a
great many, and it would be a shame to have to go through them more
than once.

I would naively expect that replacing fmgr calls with direct calls
would lead to a performance gain (no fmgr overhead), but honestly I'm
not sure whether that would actually make a difference.

Thanks for your time,
BJ

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Brendan Jurd" <direvus@gmail.com> 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

On Wed, Mar 19, 2008 at 12:51:35PM -0400, Tom Lane wrote:

"Brendan Jurd" <direvus@gmail.com> writes:

char * text_cstring(const text *t)

What do people think of text_to_cstring?

I tend to put things the other way around in my code, i.e:

char * cstring_of_text(const text *t)

mainly because things read more easily---type definitions of new
variables are next to the first part of the word.

char * str = cstring_of_text(src_text);
vs.
char * str = text_to_cstring(src_text);

I think I got my original inspiration for doing it this way around from
the Caml language.

Sam

On Wed, 19 Mar 2008, Sam Mason <sam@samason.me.uk> writes:

...
char * str = cstring_of_text(src_text);
...

I think I got my original inspiration for doing it this way around from
the Caml language.

Also, used in Common Lisp as class accessors:

char *s = cstring_of(text);
text *t = text_of(cstring);

But I'd vote for TextPGetCString style Tom suggested for the eye-habit
compatibility with the rest of the code.

Regards.

Volkan YAZICI <yazicivo@ttmail.com> writes:

But I'd vote for TextPGetCString style Tom suggested for the eye-habit
compatibility with the rest of the code.

If there are not additional votes, I'll go with TextPGetCString
and CStringGetTextP.

regards, tom lane

On 20/03/2008, Tom Lane <tgl@sss.pgh.pa.us> 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

"Brendan Jurd" <direvus@gmail.com> 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