Removing GlobalVisTestNonRemovableHorizon

  • Jump to comment-1
    Andres Freund<andres@anarazel.de>
    Apr 15, 2024, 6:57 PM UTC
    Hi,
    GlobalVisTestNonRemovableHorizon()/GlobalVisTestNonRemovableFullHorizon() only
    existed for snapshottooold - but that was removed in f691f5b80a8. I'm
    inclined to think we should remove those functions for 17. No new code should
    use them.
    Greetings,
    Andres Freund
    • Jump to comment-1
      Michael Paquier<michael@paquier.xyz>
      Apr 15, 2024, 10:33 PM UTC
      On Mon, Apr 15, 2024 at 11:57:20AM -0700, Andres Freund wrote:
      GlobalVisTestNonRemovableHorizon()/GlobalVisTestNonRemovableFullHorizon() only
      existed for snapshottooold - but that was removed in f691f5b80a8. I'm
      inclined to think we should remove those functions for 17. No new code should
      use them.
      RMT hat on. Feel free to go ahead and clean up that now. No
      objections from here as we don't want to take the risk of this stuff
      getting more used in the wild.
      --
      Michael
      • Jump to comment-1
        Andres Freund<andres@anarazel.de>
        Apr 17, 2024, 7:42 PM UTC
        On 2024-04-16 07:32:55 +0900, Michael Paquier wrote:
        On Mon, Apr 15, 2024 at 11:57:20AM -0700, Andres Freund wrote:
        GlobalVisTestNonRemovableHorizon()/GlobalVisTestNonRemovableFullHorizon() only
        existed for snapshottooold - but that was removed in f691f5b80a8. I'm
        inclined to think we should remove those functions for 17. No new code should
        use them.

        RMT hat on. Feel free to go ahead and clean up that now. No
        objections from here as we don't want to take the risk of this stuff
        getting more used in the wild.
        Cool. Pushed the removal..
    • Jump to comment-1
      Robert Haas<robertmhaas@gmail.com>
      Apr 15, 2024, 7:14 PM UTC
      On Mon, Apr 15, 2024 at 2:57 PM Andres Freund <andres@anarazel.de> wrote:
      GlobalVisTestNonRemovableHorizon()/GlobalVisTestNonRemovableFullHorizon() only
      existed for snapshottooold - but that was removed in f691f5b80a8. I'm
      inclined to think we should remove those functions for 17. No new code should
      use them.
      +1 for removing whatever people shouldn't be using. I recently spent a
      lot of time looking at this code and it's quite complicated and hard
      to understand. It would of course have been nice to have done this
      sooner, but I don't think waiting for next release cycle will make
      anything better.
      --
      Robert Haas
      EDB: http://www.enterprisedb.com
      • Jump to comment-1
        Andres Freund<andres@anarazel.de>
        Apr 17, 2024, 7:41 PM UTC
        Hi,
        On 2024-04-15 15:13:51 -0400, Robert Haas wrote:
        It would of course have been nice to have done this sooner, but I don't
        think waiting for next release cycle will make anything better.
        I don't really know how it could have been discovered sooner. We don't have
        any infrastructure for finding code that's not used anymore. And even if we
        had something finding symbols that aren't referenced within the backend, we
        have lots of functions that are just used by extensions, which would thus
        appear unused.
        In my local build we have several hundred functions that are not used within
        the backend, according to -Wl,--gc-sections,--print-gc-sections. A lot of that
        is entirely expected stuff, like RegisterXactCallback(). But there are also
        long-unused things like TransactionIdIsActive().
        Greetings,
        Andres Freund