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.
> 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
> 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 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 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.
> 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
:)
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.
--
Michael
