Re: Moving other hex functions to /common

On Tue, Jan 5, 2021 at 03:47:59PM +0900, Michael Paquier wrote:
> On Mon, Jan 04, 2021 at 10:47:39PM -0500, Bruce Momjian wrote:
> > I can see the value of passing the destination length to the hex
> > functions, and I think you have to pass the src length to pg_hex_encode
> > since the input can be binary. I assume the pg_hex_decode doesn't need
> > the source length because it is a null-terminated C string, right?
>
> I don't think that we should assume that this is always the case,
> actually. That would be an assumption easy to miss for callers of
> those routines.

Well, if the backend uses /common for hex like I suggested, and like we
do now, it has to match the function signatures of bytea and esc, see
struct pg_encoding. I don't see the point in changing those.

> > However, pg_base64_decode(const char *src, size_t len, char *dst) does
> > pass in the src length, so I can see we should just make it the same. I

Sorry, I was wrong in the above --- len is the destination length.

> > also agree it is unclear if 'len' is the src or dst len, so your patch
> > fixes that with the names. Also, is there a reason you use int64
> > instead of the size_t used by bytea?
>
> For the argument type, sure you could just use size_t, using an int
> just looked more consistent to me to match with the style of
> base64.c.

I think we would more likely match what adt/encoding.c does, and we
actually must if we are going to use /common for the backend.

> > I don't think removing the two error type reporting from src/common, and
> > then having to add it back into adt/encode.c makes sense. There are
> > two, soon three, that want those two error reports, so I am thinking we
> > are best to just leave ecpg alone since it is just a library that
> > doesn't want more than one error reporting. I don't think we will have
> > many more library call cases.
>
> Hmm. Even for libpq? I have grown allergic to the addition of more
> "ifdef FRONTEND" and elog()/pg_log() calls into src/common/.

I can understand the allergic reaction, but the other options seem
worse, and we have multiple places that need that multiple error string
reporting.

> > I think your patch has a few mistakes. First, I think you removed the
> > hex_decode files from the file system, rather than removing it via git,
> > so the diff didn't apply in the cfbot.
>
> Oh, indeed, thanks. I missed that the patch should have used
> /dev/null for the removed files.
>
> > Second, I don't think ecpg ever used libpgcommon before.
>
> Yep. That would be the first time. I don't see anything that would
> prevent its use, though I may be missing something.

The cfbot is all green for this patch now, so we are making progress. ;-)

> > For non-Windows, libpgcommon got referenced
> > anyway in the build, so non-Windows compiles worked, but in Windows,
> > that was not referenced, meaning the cfbot failed on ecpglib. I have
> > modified the attached patch with both of these fixed --- let's see how
> > it likes this version.
>
> Indeed. Likely I am to blame for not having my Windows machine at
> hand these days. I'll have this environment available only next week
> :)

Well, the cfbot gives us all access to Windows compiles, so we are good.

> FWIW, I think that this refactoring has more value iff we are able to
> remove all the duplicate hex implementations we have in the tree,
> while being able to cover the case you are looking for frontend tools
> able to do logging. Merging ECPG and the backend requires switching
> to a logic where we return more than one error code so we could just
> use an enum for the result result à-la-PQping like in libpq.

I think we are best leaving ecpglib alone, since it is a library, and
just have one other hex implementation in /common for all other cases.

I found serious confusion in encoding.c:

* function pointer prototype argument names didn't match function
prototypes
* used dlen for datalen, and data where src was used in real functions
* used len for src and dst len
* used dstlen for srclen

It was very confusing, and this attached patch fixes all of that. I
also added the pg_ prefix you suggrested. If we want to add dstlen to
all the functions, we have to do it for all types --- not sure it is
worth it, now that things are much clearer.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee