From: | Bruce Momjian <bruce(at)momjian(dot)us> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Moving other hex functions to /common |
Date: | 2021-01-05 17:21:09 |
Message-ID: | 20210105172109.GH7432@momjian.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
hex.diff.gz | application/gzip | 3.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2021-01-05 17:22:16 | Re: Moving other hex functions to /common |
Previous Message | Michael Banck | 2021-01-05 17:19:31 | Re: Online checksums patch - once again |