Size vs size_t or, um, PgSize?
Hi,
I've run into an issue of a name clash with system libraries. Specifically,
the `Size` type seems to be just an alias for `size_t` and, at least on
macOS, it clashes with the core SDK, as it is also defined by MacTypes.h,
which is used by some of the libraries one may want to use from within a
Postgres extension.
While in my case, I believe I have a workaround, I couldn't find any
rationale as to why we might want to have this alias and not use size_t.
Any insight on this would be appreciated.
Would there be any sense in changing it all to size_t or renaming it to
something else?
I understand that they will break some extensions, so if we don't want them
to have to go through with the renaming, can we enable backward
compatibility with a macro?
If there's a willingness to try this out, I am happy to prepare a patch.
--
Y.
On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
I couldn't find any rationale as to why we might want to have this alias and not use size_t. Any insight on this would be appreciated.
This used to be a typedef for unsigned int a very long time ago.
Would there be any sense in changing it all to size_t or renaming it to something else?
I understand that they will break some extensions, so if we don't want them to have to go through with the renaming, can we enable backward compatibility with a macro?
If there's a willingness to try this out, I am happy to prepare a patch.
This has been discussed a number of times in the past, and the conclusion from
last time IIRC was to use size_t for new code and only change the existing
instances when touched for other reasons to avoid churn.
--
Daniel Gustafsson
On Tue, Jul 4, 2023 at 6:46 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
If there's a willingness to try this out, I am happy to prepare a patch.This has been discussed a number of times in the past, and the conclusion from
last time IIRC was to use size_t for new code and only change the existing
instances when touched for other reasons to avoid churn.
One such earlier discussion:
/messages/by-id/CAEepm=1eA0vsgA7-2oigKzqg10YeXoPWiS-fCuQRDLwwmgMXag@mail.gmail.com
I personally wouldn't mind if we just flipped to standard types
everywhere, but I guess it wouldn't help with your problem with
extensions on macOS as you probably also want to target released
branches, not just master/17+. But renaming in the back branches
doesn't sound like something we'd do...
Hi Thomas,
On Mon, Jul 3, 2023 at 12:03 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Tue, Jul 4, 2023 at 6:46 AM Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
If there's a willingness to try this out, I am happy to prepare apatch.
This has been discussed a number of times in the past, and the
conclusion from
last time IIRC was to use size_t for new code and only change the
existing
instances when touched for other reasons to avoid churn.
One such earlier discussion:
/messages/by-id/CAEepm=1eA0vsgA7-2oigKzqg10YeXoPWiS-fCuQRDLwwmgMXag@mail.gmail.com
I personally wouldn't mind if we just flipped to standard types
everywhere, but I guess it wouldn't help with your problem with
extensions on macOS as you probably also want to target released
branches, not just master/17+. But renaming in the back branches
doesn't sound like something we'd do...
Of course, it would have been great to have it backported in the ideal
world, but it isn't realistic, as you say.
That being said, going ahead with the global renaming of Size to size_t
will mostly eliminate this clash in, say, five years when old versions will
be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at
all. To me, having the problem gone in the future beats having the problem
forever.
--
Y.
On 3 Jul 2023, at 21:14, Yurii Rashkovskii <yrashk@gmail.com> wrote:
That being said, going ahead with the global renaming of Size to size_t will mostly eliminate this clash in, say, five years when old versions will be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at all. To me, having the problem gone in the future beats having the problem forever.
I would also like all Size instances gone, but the cost during backpatching
will likely be very high. There are ~1300 or so of them in the code, and
that's a lot of potential conflicts during the coming 5 years of backpatches.
--
Daniel Gustafsson
Daniel,
On Mon, Jul 3, 2023 at 12:20 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 3 Jul 2023, at 21:14, Yurii Rashkovskii <yrashk@gmail.com> wrote:
That being said, going ahead with the global renaming of Size to size_t
will mostly eliminate this clash in, say, five years when old versions will
be gone. At least it'll be fixed then. Otherwise, it'll never be fixed at
all. To me, having the problem gone in the future beats having the problem
forever.I would also like all Size instances gone, but the cost during backpatching
will likely be very high. There are ~1300 or so of them in the code, and
that's a lot of potential conflicts during the coming 5 years of
backpatches.
I understand. How about a workaround for extension builders? Something like
```
/* Use this if you run into Size type redefinition */
#ifdef DONT_TYPEDEF_SIZE
#define Size size_t
#else
typedef size_t Size;
#endif
```
This way, extension developers can specify DONT_TYPEDEF_SIZE. However, this
would have to be backported, but to minimal/no effect if I am not missing
anything.
Not beautiful, but better than freezing the status quo forever?
--
Y.
Daniel Gustafsson <daniel@yesql.se> writes:
On 3 Jul 2023, at 20:32, Yurii Rashkovskii <yrashk@gmail.com> wrote:
I couldn't find any rationale as to why we might want to have this alias and not use size_t. Any insight on this would be appreciated.
This used to be a typedef for unsigned int a very long time ago.
I'm fairly sure that Size dates from before we could expect the system
headers to provide size_t everywhere.
This has been discussed a number of times in the past, and the conclusion from
last time IIRC was to use size_t for new code and only change the existing
instances when touched for other reasons to avoid churn.
Yeah. The code-churn costs of s/Size/size_t/g outweigh the possible
gain, at least from our admittedly project-centric point of view.
But I don't have a whole lot of sympathy for arguments about "this
other code I'd like to also use has its own definition for Size",
because you could potentially make that complaint about just about
every typedef we've got. If you have conflicts like that, you have
to resolve them by methods like #define hacks or factoring your code
so it doesn't need to include Postgres headers in the same files that
include $other-project-headers.
regards, tom lane